Change actions.txt to actions.xml

tools/metrics/actions/extract_actions_test.py

Index: tools/metrics/actions/extract_actions_test.py
diff --git a/components/tools/metrics/count_ifdefs_unittest.py b/tools/metrics/actions/extract_actions_test.py
similarity index 16%
copy from components/tools/metrics/count_ifdefs_unittest.py
copy to tools/metrics/actions/extract_actions_test.py
index 9b8c3229a0f625b6eb0d3bd2b49982cb93d4e74a..1fef187d81db45a23bc798579cd532c82af58a44 100755
--- a/components/tools/metrics/count_ifdefs_unittest.py
+++ b/tools/metrics/actions/extract_actions_test.py
@@ -1,29 +1,42 @@
 #!/usr/bin/env python
-# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Copyright (c) 2014 The Chromium Authors. All rights reserved.

[Ilya Sherman, 2014/02/26 01:32:18]

Ultra nit: Just "Copyright 2014" (i.e. no "(c)")

[yao, 2014/02/27 14:45:21]

Done.

 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""Tests for count_ifdefs.
-"""
-
-import os
+import tempfile
 import unittest
 
-import count_ifdefs
+import extract_actions
 
 
-class CountIfdefsTest(unittest.TestCase):
+ACTIONS_MOCK = """
+<actions>
 
-  def setUp(self):
-    self.root = os.path.join(os.path.dirname(__file__), 'testdata')
+<action name="AboutChrome">
+  <owner>name1@google.com</owner>
+  <owner>name2@google.com</owner>
+  <description>Description.</description>
+</action>

[Ilya Sherman, 2014/02/26 01:32:18]

Please test edge cases as well.  What happens if there is just one <owner> tag? 
What about 0?  What happens if there is an <obsolete> tag?  What happens if a
required tag is missing, or duplicated?  What happens if a tag's value is empty?
 What formatting requirements are there for the contents of an <owner> tag?

[yiyaoliu, 2014/03/03 20:44:22]

Edge cases like 0, 1, or more <owner>, <description> and <obsolete> tags are
tested. 
Currently, there's format requirement for <owner> tag and we'll make it pass if
the a tag value is empty. 

[yiyaoliu, 2014/03/03 20:48:25]

Sorry typo, there's no format requirement for <owner> tag.

+
+</actions>
+"""
 
-  def testNormal(self):
-    count = count_ifdefs.CountIfdefs('OS_[A-Z]+', self.root)
-    self.failUnless(count == 6)
 
-  def testSkipTests(self):
-    count = count_ifdefs.CountIfdefs('OS_[A-Z]+', self.root, True)
-    self.failUnless(count == 4)
+class ActionXmlTest(unittest.TestCase):
+
+  def testReserveValue(self):
+    with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
+      f.write(ACTIONS_MOCK)
+      file_name = f.name
+    actions, actions_dict = extract_actions._ParseActionFile(file_name)

[Ilya Sherman, 2014/02/26 01:32:18]

Rather than testing the internal function, it's most helpful to test the
effective public API.  In this case, I think that would essentially be testing
that the system is able to start with an actions.xml file, slurp it in, add some
automatically aggregated actions, and write it out... plus doing some data
validation on the side.

[yao, 2014/02/27 14:45:21]

Could you give me a more concrete idea of how you want it to be tested? 

Since most of the function in main are either hard coded (like
AddAboutFlagsActions) or dependent on the file system. It wound't easy to test
them as a whole, cause the code base can always be changed and we won't be able
to know which action will be there in the future. 

In the main function, I only added 2 functions, _ParseActionFile and
_PrettyPrint. I feel these 2 functions are relatively public. If it's the
starting underscore in the function name that makes you uncomfortable, it could
be easily changed. 

The previous comment will be addressed after I understand how test should be
done. 

[yao, 2014/02/27 14:50:55]

Actually I agree that if a function will be used anywhere else outside of the
file it's defined, the function name shouldn't start with an underscore. 
Will wait for your opinion before I change things. 

[Ilya Sherman, 2014/03/01 00:58:14]

I'm not going to be too picky about test coverage -- some is better than none,
and I don't want to block this CL indefinitely.  I'll defer to Alexei's
judgement as to what level of testing is enough for now.

But, if you wanted to pursue testing along the lines that I described, I think
what you'd need is some way to mock out the input that the extract_actions.py
script runs over.  Currently, that's essentially hardcoded to be the files in
the repository under the 'src' directory.  The code would be much more testable
-- and probably easier to integrate with PRESUBMIT.py as well -- if it took an
input iterator over file- or string-like objects that it could run the various
grep steps over.

+    self.assertEqual(1, len(actions))
+    self.assertEqual(1, len(actions_dict))
+    self.assertIn('AboutChrome', actions)
+    self.assertIn('AboutChrome', actions_dict)
+    self.assertEqual(2, len(actions_dict['AboutChrome'].owners))
+    self.assertIn('name1@google.com', actions_dict['AboutChrome'].owners)
+    self.assertIn('name2@google.com', actions_dict['AboutChrome'].owners)
+    self.assertEqual('Description.', actions_dict['AboutChrome'].description)
 
 
 if __name__ == '__main__':