Change actions.txt to actions.xml

tools/metrics/actions/extract_actions_test.py

Index: tools/metrics/actions/extract_actions_test.py
diff --git a/tools/metrics/actions/extract_actions_test.py b/tools/metrics/actions/extract_actions_test.py
new file mode 100755
index 0000000000000000000000000000000000000000..9c0e1f91f18185c5f148fcd9b764755ecb0b8080
--- /dev/null
+++ b/tools/metrics/actions/extract_actions_test.py
@@ -0,0 +1,184 @@
+#!/usr/bin/env python
+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import tempfile
+import unittest
+
+import extract_actions
+
+# Empty value to be inserted to |ACTIONS_MOCK|.
+NO_VALUE = ''
+
+ONE_OWNER = '<owner>name1@google.com</owner>\n'
+TWO_OWNERS = """
+<owner>name1@google.com</owner>\n
+<owner>name2@google.com</owner>\n
+"""
+
+DESCRIPTION = '<description>Description.</description>\n'
+TWO_DESCRIPTIONS = """
+<description>Description.</description>\n
+<description>Description2.</description>\n
+"""
+
+OBSOLETE = '<obsolete>Not used anymore. Replaced by action2.</obsolete>\n'
+TWO_OBSOLETE = '<obsolete>Obsolete1.</obsolete><obsolete>Obsolete2.</obsolete>'
+
+COMMENT = '<!--comment-->'
+
+ACTIONS_XML = """

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

This should either have a more meaningful name (e.g. call it a format string) or
at least a comment.

[yiyaoliu, 2014/03/04 18:02:54]

Done.

+{comment}
+<actions>
+
+<action name="action1">\n{owners}{obsolete}{description}

[Ilya Sherman, 2014/03/03 23:17:34]

nit: Please leave a blank line after the \n, unless that breaks expectations
somehow.

[yiyaoliu, 2014/03/04 15:31:07]

Done.

+</action>
+
+</actions>"""
+
+NO_OWNER_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>Please list the metric\'s owners.'
+    ' Add more owner tags as needed.</owner>\n'

[Ilya Sherman, 2014/03/03 23:17:34]

nit: Odd indentation on this line.

[yiyaoliu, 2014/03/04 15:31:07]

I put the blank up to the line above. Is this what you meant?

+    '  <description>Description.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+ONE_OWNER_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <description>Description.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+TWO_OWNERS_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <owner>name2@google.com</owner>\n'
+    '  <description>Description.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+NO_DESCRIPTION_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <owner>name2@google.com</owner>\n'
+    '  <description>Please enter the description of the metric.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+OBSOLETE_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <owner>name2@google.com</owner>\n'
+    '  <description>Description.</description>\n'
+    '  <obsolete>Not used anymore. Replaced by action2.</obsolete>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+ADD_ACTION_EXPECTED_XML = (
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <owner>name2@google.com</owner>\n'
+    '  <description>Description.</description>\n'
+    '</action>\n\n'
+    '<action name="action2">\n'
+    '  <owner>Please list the metric\'s owners.'
+    ' Add more owner tags as needed.</owner>\n'
+    '  <description>Please enter the description of the metric.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+COMMENT_EXPECTED_XML = (
+    '<!--comment-->\n\n'
+    '<actions>\n\n'
+    '<action name="action1">\n'
+    '  <owner>name1@google.com</owner>\n'
+    '  <owner>name2@google.com</owner>\n'
+    '  <description>Description.</description>\n'
+    '</action>\n\n'
+    '</actions>\n'
+)
+
+
+class ActionXmlTest(unittest.TestCase):
+
+  def _assertActionResult(self, owner, description, obsolete, expected_result,

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

Please add some comments for this method to explain the params and what it does.

[yiyaoliu, 2014/03/04 18:02:54]

Done.

+                          new_action=None, comment=NO_VALUE):
+    current_xml = ACTIONS_XML.format(owners=owner, description=description,
+                                     obsolete=obsolete, comment=comment)
+    with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

How hard would it be to expose a extract_actions.ParseActionString(string)
method from the implementation file, so that the tests don't have to bother with
this tempfile stuff?

[yiyaoliu, 2014/03/04 18:02:54]

It's actually better to pass in a string. Changed as you suggested. 

+      f.write(current_xml)
+      file_name = f.name
+    actions, actions_dict, comments = extract_actions.ParseActionFile(file_name)
+    if new_action:
+      actions.add(new_action)
+    pretty = extract_actions.PrettyPrint(actions, actions_dict, comments)
+    self.assertEqual(expected_result, pretty)

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

I think it's clearer to have this assert line be in the test bodies themselves
and have this function simply return |pretty|.

Then, it would be more straightforward to understand what the tests are doing,
e.g.

  xml_result = self._ParseAndFormatXML(...)
  self.assertEqual(NO_OWNER_EXPECTED_XML, xml_result)

[yiyaoliu, 2014/03/04 18:02:54]

Done.

+
+  def testNoOwner(self):
+    self._assertActionResult(NO_VALUE, DESCRIPTION, NO_VALUE,
+                             NO_OWNER_EXPECTED_XML)
+
+  def testOneOwnerOneDescription(self):
+    self._assertActionResult(ONE_OWNER, DESCRIPTION, NO_VALUE,
+                             ONE_OWNER_EXPECTED_XML)
+
+  def testTwoOwners(self):
+    self._assertActionResult(TWO_OWNERS, DESCRIPTION, NO_VALUE,
+                             TWO_OWNERS_EXPECTED_XML)
+
+  def testNoDescription(self):
+    self._assertActionResult(TWO_OWNERS, NO_VALUE, NO_VALUE,
+                             NO_DESCRIPTION_EXPECTED_XML)
+
+  def testTwoDescriptions(self):
+    current_xml = ACTIONS_XML.format(owners=TWO_OWNERS, obsolete=NO_VALUE,
+                                     description=TWO_DESCRIPTIONS,
+                                     comment=NO_VALUE)
+    with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
+      f.write(current_xml)
+      file_name = f.name
+    with self.assertRaises(SystemExit) as cm:
+      _, _ = extract_actions.ParseActionFile(file_name)
+    self.assertEqual(cm.exception.code, 1)
+
+  def testObsolete(self):
+    self._assertActionResult(TWO_OWNERS, DESCRIPTION, OBSOLETE,
+                             OBSOLETE_EXPECTED_XML)
+
+  def testTwoObsoletes(self):
+    current_xml = ACTIONS_XML.format(owners=TWO_OWNERS, obsolete=TWO_OBSOLETE,
+                                     description=DESCRIPTION,
+                                     comment=NO_VALUE)
+    with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

Seems this logic is common between several places. Can you make a separate
helper method for this? (i.e. the formatting above and the tempfile stuff)

+      f.write(current_xml)
+      file_name = f.name
+    with self.assertRaises(SystemExit) as cm:

[Alexei Svitkine (slow), 2014/03/04 15:32:13]

Can you add a comment explaining what this is doing?

+      _, _ = extract_actions.ParseActionFile(file_name)
+    self.assertEqual(cm.exception.code, 1)
+
+  def testAddNewActions(self):
+    self._assertActionResult(TWO_OWNERS, DESCRIPTION, NO_VALUE,
+                             ADD_ACTION_EXPECTED_XML, new_action='action2')
+
+  def testComment(self):
+    self._assertActionResult(TWO_OWNERS, DESCRIPTION, NO_VALUE,
+                             COMMENT_EXPECTED_XML, comment=COMMENT)
+
+
+if __name__ == '__main__':
+  unittest.main()