Merge lp:~joetalbott/utah/add_testcase_docs_to_result into lp:utah

[Joe Talbott]

This adds the testcase documentation to the result packet.

The following fields are now required in tc_control
- dependencies
- action
- expected_results

[Javier Collado (javier.collado)]

@Joe

After looking at the changes, I've got a couple of comments

- extra_info

The extra_info dictionary is created using the fields from self.{description,dependencies,action,expected_results}, but I don't see any of those fields having a default value of being explicitly assigned. I guess, that they're set in the following line:

self.__dict__.update(control_data)

If control_data is not None, those fields are going to be available because of the schema validation. However, when control_data is None that line isn't executed and I'm not sure about what happens then. Is it possible to call TestCase.run in such a case? If that might happen, then an exception will be raised when those variables aren't found.

By the way, self.__dict__ is an implementation detail and updating it directly isn't considered a good practice. I think that I'd prefer to use internally self.control_data[<field_name>] rather than self.<field_name> to avoid the confusion I had (and others might have as well in the future) about were all those instance variables are assigned. If you like to keep using the object dictionary directly, isn't a big deal, but a comment about that could be added to the code.

An alternative code to avoid using self.__dict__ direclty would be as follows:
for key, value in control_data.iteritems():
    setattr(self, key, value)

-static_analysis

Since you said you are interested in running static analysis in your code, I can tell you that there are two errors in testcase.py
- Line 210: W293 blank line contains whitespace
- Line 212: E303 too many blank lines (2)

review: Needs Information

[lp:~joetalbott/utah/add_testcase_docs_to_result]

690. By Joe Talbott on 2012-10-04

Ensure that the testcase documentation is provided * add testcases

[Joe Talbott (joetalbott)]

@Javier

I've updated the merge proposal to address your issues.

review: Needs Resubmitting

[lp:~joetalbott/utah/add_testcase_docs_to_result]

691. By Joe Talbott on 2012-10-04

debian/changelog - Fix conflicts for merge.

[Javier Collado (javier.collado)]

Looks good to me know. Thanks for the update.

review: Approve

=== modified file 'utah/client/exceptions.py'
--- utah/client/exceptions.py	2012-08-28 11:22:41 +0000
+++ utah/client/exceptions.py	2012-10-04 15:47:24 +0000
@@ -40,3 +40,10 @@
     """
     Used to provide additional information when schema validation fails
     """
+
+
+class MissingData(UTAHClientError):
+    """
+    Raised when there is missing data that is required for an object
+    to proceed.
+    """

=== modified file 'utah/client/result.py'
--- utah/client/result.py	2012-09-06 15:44:36 +0000
+++ utah/client/result.py	2012-10-04 15:47:24 +0000
@@ -60,7 +60,7 @@
         self.failures = 0
         self.passes = 0
 
-    def add_result(self, result):
+    def add_result(self, result, extra_info={}):
         """
         Add a result to the object.
 
@@ -78,6 +78,10 @@
         if result is None:
             return
 
+        # Add items from extra_info into the result
+        if len(extra_info) > 0:
+            result['extra_info'] = extra_info
+
         if self.testsuite is not None:
             result['testsuite'] = self.testsuite
 

=== modified file 'utah/client/testcase.py'
--- utah/client/testcase.py	2012-09-27 13:55:41 +0000
+++ utah/client/testcase.py	2012-10-04 15:47:24 +0000
@@ -6,7 +6,7 @@
 from common import run_cmd, parse_control_file
 from common import do_nothing
 from common import CMD_TC_BUILD, CMD_TC_SETUP, CMD_TC_TEST, CMD_TC_CLEANUP
-from exceptions import MissingFile, ValidationError
+from exceptions import MissingFile, ValidationError, MissingData
 
 
 class TestCase(object):
@@ -32,6 +32,18 @@
                 'type': 'string',
                 'required': True,
                 },
+            'dependencies': {
+                'type': 'string',
+                'required': True,
+                },
+            'action': {
+                'type': 'string',
+                'required': True,
+                },
+            'expected_results': {
+                'type': 'string',
+                'required': True,
+                },
             'type': {
                 'type': 'string',
                 'enum': ['userland']
@@ -70,6 +82,13 @@
         self.reboot_callback = _reboot_callback
         self.run_as = None
 
+        # The following are testcase documentation and should all be provided
+        # in a tc_control file or via the state file.
+        self.description = None
+        self.dependencies = None
+        self.action = None
+        self.expected_results = None
+
         if _save_state_callback is not None:
             self.save_state_callback = _save_state_callback
 
@@ -94,7 +113,8 @@
             if self.timeout is not None:
                 control_data['timeout'] = self.timeout
 
-            self.__dict__.update(control_data)
+            for key, value in control_data.iteritems():
+                setattr(self, key, value)
 
         # Set any values passed into __init__() as final values
         if timeout is None and self.timeout is None:
@@ -150,6 +170,13 @@
             self.run_status = 'PASS'
             return self.run_status
 
+        # Do not proceed if we are missing data
+        if (self.description is None
+                or self.dependencies is None
+                or self.action is None
+                or self.expected_results is None):
+                raise MissingData
+
         # Return value to indicate whether processing of a TestSuite should
         # continue.  This is to avoid a shutdown race on reboot cases.
         keep_going = True
@@ -182,12 +209,19 @@
             timeout = self.timeout or 0
             self.status = "RUN"
             self.save_state_callback()
+            extra_info = {
+                    'description': self.description,
+                    'dependencies': self.dependencies,
+                    'action': self.action,
+                    'expected_results': self.expected_results,
+                    }
             result.add_result(run_cmd(self.command,
                                       timeout=timeout,
                                       cwd=self.working_dir,
                                       cmd_type=CMD_TC_TEST,
                                       run_as=self.run_as,
-                                      ))
+                                      ),
+                                      extra_info=extra_info)
 
         # Clean up whether 'command' failed or not.
         self.cleanup(result)
@@ -224,7 +258,8 @@
 
         Requires that 'state' has the same fieldnames as the TestCase class.
         """
-        self.__dict__.update(state)
+        for key, value in state.iteritems():
+            setattr(self, key, value)
 
     def save_state(self):
         """
@@ -241,6 +276,9 @@
                 'tc_cleanup': self.tc_cleanup,
                 'type': self.type,
                 'description': self.description,
+                'dependencies': self.dependencies,
+                'action': self.action,
+                'expected_results': self.expected_results,
         }
 
         return state

=== modified file 'utah/client/tests/test_testcase.py'
--- utah/client/tests/test_testcase.py	2012-08-08 16:17:12 +0000
+++ utah/client/tests/test_testcase.py	2012-10-04 15:47:24 +0000
@@ -1,6 +1,7 @@
 import os
 import unittest
 
+from utah.client.exceptions import MissingData
 from utah.client.result import ResultYAML
 from utah.client.common import (
         UTAH_DIR,
@@ -90,3 +91,53 @@
         )
 
         self.assertEqual(timeout, case.timeout)
+
+    def test_control_data(self):
+        """
+        Test that a correctly formed control_data that's passed in
+        doesn't raise an exception.
+        """
+        control_data = {
+                'description': 'a test case',
+                'command': '/bin/true',
+                'dependencies': 'n/a',
+                'action': 'n/a',
+                'expected_results': 'n/a',
+                }
+
+        result = ResultYAML()
+
+        case = testcase.TestCase(
+                name=self.name,
+                path=self.path,
+                result=result,
+                _control_data=control_data,
+                )
+
+        case.run()
+        print result
+        result.result()
+
+        self.assertTrue(case.is_done())
+
+    def test_bad_control_data(self):
+        """
+        Test that an incorrectly formed control_data that's passed in
+        does raise an exception.
+        """
+        control_data = {
+                'description': 'a test case',
+                'command': '/bin/true',
+                }
+
+        result = ResultYAML()
+
+        case = testcase.TestCase(
+                name=self.name,
+                path=self.path,
+                result=result,
+                _control_data=control_data,
+                )
+
+        with self.assertRaises(MissingData):
+            case.run()