Merge lp:~roadmr/checkbox/1187216-check-job-format into lp:checkbox

[Daniel Manrique]

Added a test that validates that manual (user-verify and user-interact included) jobs contain the verification field in their description, without which the Qt UI tends to skip tests.

Added a test that validates that manual (user-verify and user-interact included) jobs contain the verification field in their description, without which the Qt UI tends to skip tests.

The test can be trivially modified to check for any other fields in the description.

With this test, builds will fail if any job is malformed, so at least we won't build bad stuff.

[Jeff Lane  (bladernr)]

Just to be pedantic:

This job description:
plugin: user-interact
name: mediacard/mmc-foo
command: do_some_stuff.exe.bat.com
_description:
 PURPOSE:
    to flob the brats
 VERIFICATION:
    blab:

Results in this failure:
FAIL: test_verify_manual_jobs_have_required_fields (tests.test_message_files.MessageFileFormatTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/bladernr/development/daniel-job-desc-test/checkbox-old/checkbox/tests/test_message_files.py", line 122, in test_verify_manual_jobs_have_required_fields
    msg.format(message['name']))
AssertionError: False is not true : Missing verification in mediacard/mmc-foo

I am actually missing the "STEPS" part, not VERIFICATION.

This description returns the same error:
plugin: user-interact
name: mediacard/mmc-foo
command: do_some_stuff.exe.bat.com
_description:
 PURPOSE:
    to flob the brats
 STEPS:
    1: Steal Underpands!
    2: ??
    3: PROFIT!

only it IS missing the VERIFICATION part...

any chance to get it to say WHAT is missing?

review: Needs Fixing

[Jeff Lane  (bladernr)]

Also, why the change to fingerprint.txt.in removing the PREREQUISITE: field?

[Daniel Manrique (roadmr)]

It's not being pedantic, it's actually a very valid concern, luckily it's something I can explain.

I run the description through the DescriptionParser, the way checkbox does. If the parser can extract the required fields (purpose, steps, verification, and (optionally) info), it calls a specified method and passes these values (the method then sets the needed variables in a context-dependent way; look at the DescriptionResult mini-class).

However, if the description is malformed (well, not "malformed" but not parsable according to expectations), the DescriptionParser doesn't do anything and the DescriptionResult will be completely empty. Programs are expected to use the raw description in this case.

The message you're getting is in essence correct; the DescriptionResult itself has no "verification" key. However this is because the test description was unparsable (i.e. it had VERIFICATION but lacks STEPS so the parser deems it entirely invalid), so the DescriptionResult object is empty.

I can certainly make the test a bit less crude; it could indicate unparsable descriptions, which *should* work, but in practice I think the Qt UI doesn't like them so we may as well catch and report them.

Removing PREREQUISITE was for the same reason: the parser doesn't know about that section identifier, so it gives up (invalid description as far as it's concerned) and returns an empty DescriptionResult.

I'll modify the test to make it cleverer and more explanatory, should have it ready soon.

[lp:~roadmr/checkbox/1187216-check-job-format]

2164. By Daniel Manrique on 2013-06-05

Changed to just run through the parser and reject unparsable descriptions

[Daniel Manrique (roadmr)]

OK, so I simplified the test and the message, it will simply say "wrong job format" if the parser returns an empty set of fields. Turns out, they're all mandatory (save for INFO) and need to be in the correct order, so this seems simpler. So it's impossible for the parser to return an incomplete set of fields.

To produce a more detailed report of what's wrong, I'd have to implement a new parser (as the existing one is not geared towards diagnostics), not sure if it's needed.

This is the sort of error it produces now:

======================================================================
FAIL: test_verify_manual_jobs_have_parsable_description (checkbox.tests.test_message_files.MessageFileFormatTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/media/bulk/checkboxes/checkbox/1187216-check-job-format/checkbox-old/checkbox/tests/test_message_files.py", line 122, in test_verify_manual_jobs_have_parsable_description
    msg.format(message['name']))
AssertionError: {} is not true : Description for manual job mediacard/mmc-foo2 has wrong format

review: Needs Resubmitting

[Jeff Lane  (bladernr)]

Thanks, that's less confusing. And Yeah, no need to write a completely new parser just to make a unit test easier :)

Thanks a lot for making those changes!

review: Approve

[Brendan Donegan (brendan-donegan)]

Don't forget to set the overall status to 'Approved' to let Tarmac do its thing

=== modified file 'checkbox-old/checkbox/tests/test_message_files.py'
--- checkbox-old/checkbox/tests/test_message_files.py	2013-05-29 07:50:30 +0000
+++ checkbox-old/checkbox/tests/test_message_files.py	2013-06-05 15:27:29 +0000
@@ -18,8 +18,20 @@
 #
 import os
 import unittest
+from io import StringIO
+
 from checkbox.lib.path import path_expand_recursive
 from checkbox.lib.template_i18n import TemplateI18n
+from checkbox.parsers.description import DescriptionParser
+
+
+class DescriptionResult(object):
+    def __init__(self):
+        self.fields = {}
+
+    def setDescription(self, purpose, steps, verification, info):
+        for name in ['purpose', 'steps', 'verification', 'info']:
+            self.fields[name] = locals()[name]
 
 
 class MessageFileFormatTest(unittest.TestCase):
@@ -72,11 +84,11 @@
                         shell_variables += re.findall(shell_variables_regex,
                                                       message[key])
                 if 'environ' in message:
-                    environ_variables = re.findall(environ_variables_regex, 
+                    environ_variables = re.findall(environ_variables_regex,
                                                    message['environ'])
                 self.assertEqual(set(environ_variables),
-                                  set(shell_variables),
-                                  message['name'])
+                                 set(shell_variables),
+                                 message['name'])
 
     def test_jobs_comply_with_schema(self):
         globals = {}
@@ -95,3 +107,17 @@
         for message in self.messages:
             if message['plugin'] in ('user-verify', 'user-interact'):
                 self.assertTrue("command" in message)
+
+    def test_verify_manual_jobs_have_parsable_description(self):
+        for message in self.messages:
+            if message['plugin'] in ('user-verify', 'user-interact', 'manual'):
+                for key in ['description', 'description_extended']:
+                    if key in message and message[key]:
+                        descstream = StringIO(message[key])
+                        parser = DescriptionParser(descstream)
+                        result = DescriptionResult()
+                        parser.run(result)
+                        msg = "Description for manual job {} has wrong format"
+                        self.assertTrue(result.fields,
+                                        msg.format(message['name']))
+

=== modified file 'checkbox-old/debian/changelog'
--- checkbox-old/debian/changelog	2013-06-05 11:56:25 +0000
+++ checkbox-old/debian/changelog	2013-06-05 15:27:29 +0000
@@ -17,6 +17,8 @@
 
   [ Daniel Manrique ]
   * jobs/stress.txt.in: fixed a few inconsistent invocations of sleep_test.
+  * checkbox/tests/test_message_files.py: Added a test to verify jobs contain
+    required fields (LP: #1187216).
 
   [ Sylvain Pineau ]
   * jobs/resource.txt.in: updated version of the package resource job command

=== modified file 'checkbox-old/jobs/fingerprint.txt.in'
--- checkbox-old/jobs/fingerprint.txt.in	2013-05-29 07:50:30 +0000
+++ checkbox-old/jobs/fingerprint.txt.in	2013-06-05 15:27:29 +0000
@@ -2,9 +2,7 @@
 name: fingerprint/login
 _description:
  PURPOSE:
-    This test will verify that a fingerprint reader will work properly for logging into your system.
- PREREQUISITES:
-    This test case assumes that there's a testing account from which test cases are run and a personal account that the tester uses to verify the fingerprint reader
+    This test will verify that a fingerprint reader will work properly for logging into your system. This test case assumes that there's a testing account from which test cases are run and a personal account that the tester uses to verify the fingerprint reader
  STEPS:
     1. Click on the User indicator on the left side of the panel (your user name).
     2. Select "Switch User Account"