Merge lp:~kissiel/checkbox/fix-1391774-required-field-in-tp into lp:checkbox

[Maciej Kisielewski]

This MR brings 'required' field to test plan unit.

Jobs matching any pattern from the 'required' field will get always get executed REGARDLESS of the selection made. This change set doesn't make any changes in existing providers, but it paves the way for whitelist -> test plan conversion.

b2b06a3 plainbox:impl:unit:testplan: add 'required' field
0b621a3 plainbox:impl:unit:testplan: add get_required_jobs_qualifier to testplan
bd0325e checkbox-ng:commands: make 'normal run' run required jobs
1c555e5 plainbxo:docs:manpages: update testplan - add 'required' field info

[Zygmunt Krynicki (zyga)]

General comment, don't include the "impl" part in plainbox commit messages.
Push stuff _earlier_ before stuff is done as any re-design changes are cheaper.
Please fix typos in commit messages below.

Please patch plainbox to run the mandatory part if plainbox run -T is used, then you probably won't have to patch checkbox-ng at all.

I want to see a design decision on what happens if a job is selected by both normal and mandatory parts. I think it should trigger a warning from the test plan validator and that this job should be impossible to de-select in checkbox-ng/gui/touch later.

Looking at the get_$newthing_qualifier() it seems like a copy-paste of the other one, perhaps we want a new private method and have all the old methods and the new method call that instead.

Please expand the manual page to include some examples. I think we should include a very useful certification example that says "if you want a test plan that works for certification purpose, please add this snippet "required: ..."

Oh, and the word required is better but I think I still prefer mandatory as it's used similarly in other products. I'd put my $0.90 on mandatory-include instead.

More reviews next week. Thanks.

Unmerged revisions

3831. By Maciej Kisielewski on 2015-06-12

plainbxo:docs:manpages: update testplan - add 'required' field info

Signed-off-by: Maciej Kisielewski <email address hidden>

3830. By Maciej Kisielewski on 2015-06-12

checkbox-ng:commands: make 'normal run' run required jobs

This patch makes checkbox-ng run required jobs when doing normal sequence.

Fixes: https://bugs.launchpad.net/plainbox-provider-checkbox/+bug/1463921

Signed-off-by: Maciej Kisielewski <email address hidden>

3829. By Maciej Kisielewski on 2015-06-12

plainbox:impl:unit:testplan: add get_required_jobs_qualifier to testplan

This patch adds testplan method to get qualifier that corresponds to jobs
pointed by the 'required' field.

Signed-off-by: Maciej Kisielewski <email address hidden>

3828. By Maciej Kisielewski on 2015-06-11

plainbox:impl:unit:testplan: add 'required' field

This patch adds 'required' field to the testplan that specifies jobs that
should ALWAYS be run, regardless of what user selects.

Signed-off-by: Maciej Kisielewski <email address hidden>

=== modified file 'checkbox-ng/checkbox_ng/commands/newcli.py'
--- checkbox-ng/checkbox_ng/commands/newcli.py	2015-06-10 14:53:49 +0000
+++ checkbox-ng/checkbox_ng/commands/newcli.py	2015-06-12 15:06:42 +0000
@@ -329,8 +329,12 @@
         # the original ordering we should just treat it as a mask and
         # use it to filter jobs from desired_job_list.
         wanted_set = frozenset(tree.selection)
+        qualifier_list = [
+            tp.get_required_jobs_qualifier() for tp in self._testplan_list]
+        required_job_list = select_jobs(
+            self.manager.state.job_list, qualifier_list)
         job_list = [job for job in self.manager.state.run_list
-                    if job in wanted_set]
+                    if job in wanted_set] + required_job_list
         self._update_desired_job_list(job_list)
 
     def export_and_send_results(self):
 
=== modified file 'plainbox/docs/manpages/plainbox-test-plan-units.rst'
--- plainbox/docs/manpages/plainbox-test-plan-units.rst	2015-03-09 13:38:05 +0000
+++ plainbox/docs/manpages/plainbox-test-plan-units.rst	2015-06-12 15:06:42 +0000
@@ -131,6 +131,15 @@
 
     When a job is both included and excluded, exclusion always takes priority.
 
+``required``:
+    A multi-line list of job identifiers or patterns matching such identifiers
+    that should always be executed.
+
+    This optional field can be used to specify the jobs that should always run.
+    This is particularly useful for specifying jobs that gather vital
+    info about the tested system, as it renders imposible to generate a report
+    with no information about system under test.
+
 ``category-overrides``:
     A multi-line list of category override statements.
 
 
=== modified file 'plainbox/plainbox/impl/unit/test_testplan.py'
--- plainbox/plainbox/impl/unit/test_testplan.py	2015-06-10 15:40:16 +0000
+++ plainbox/plainbox/impl/unit/test_testplan.py	2015-06-12 15:06:42 +0000
@@ -104,6 +104,17 @@
         }, provider=self.provider)
         self.assertEqual(unit.exclude, "exclude")
 
+    def test_required_default(self):
+        unit = TestPlanUnit({
+        }, provider=self.provider)
+        self.assertEqual(unit.required, None)
+
+    def test_required_normal(self):
+        unit = TestPlanUnit({
+            'required': 'required'
+        }, provider=self.provider)
+        self.assertEqual(unit.required, "required")
+
     def test_category_override__default(self):
         unit = TestPlanUnit({
         }, provider=self.provider)
@@ -224,6 +235,7 @@
         # +4  # nothing
         # +5  b.*
         # +6 exclude: bar
+        # +7 required: baz
         # Let's also assume that it is at a +10 offset in the file it comes
         # from so that the first line +0 is actually the 10th Line
         src = mock.Mock(name='source', spec_set=ITextSource)
@@ -232,7 +244,8 @@
             'unit': 0,
             'name': 1,
             'include': 3,
-            'exclude': 6
+            'exclude': 6,
+            'required': 7
         }
         unit = TestPlanUnit({
             'unit': 'test-plan',
@@ -242,7 +255,8 @@
                 '# nothing\n'
                 'b.*\n'
                 ),
-            'exclude': 'bar\n'
+            'exclude': 'bar\n',
+            'required': 'baz\n'
         }, provider=self.provider, origin=origin,
             field_offset_map=field_offset_map)
         qual_list = unit.get_qualifier().get_primitive_qualifiers()
@@ -261,6 +275,11 @@
         self.assertEqual(qual_list[2].matcher.value, 'ns::bar')
         self.assertEqual(qual_list[2].origin, Origin(src, 16, 16))
         self.assertEqual(qual_list[2].inclusive, False)
+        self.assertEqual(qual_list[3].field, 'id')
+        self.assertIsInstance(qual_list[3].matcher, OperatorMatcher)
+        self.assertEqual(qual_list[3].matcher.value, 'ns::baz')
+        self.assertEqual(qual_list[3].origin, Origin(src, 17, 17))
+        self.assertEqual(qual_list[3].inclusive, True)
 
     def test_get_qualifier__only_comments(self):
         unit = TestPlanUnit({
@@ -315,3 +334,15 @@
         unit = TestPlanUnit({}, provider=self.provider)
         with self.assertRaisesRegex(ValueError, "expected override value"):
             unit.parse_category_overrides('apply')
+
+    def test_get_required_jobs_qualifier__when_required_excluded(self):
+        tp = TestPlanUnit({
+            'required': 'required',
+            'exclude': 'required'
+        }, provider=self.provider)
+        src = mock.Mock(name='source', spec_set=ITextSource)
+        qualifiers = (tp.get_required_jobs_qualifier()
+                       .get_primitive_qualifiers())
+        self.assertEqual(qualifiers[0].field, 'id')
+        self.assertIsInstance(qualifiers[0].matcher, OperatorMatcher)
+        self.assertEqual(qualifiers[0].matcher.value, 'ns::required')
 
=== modified file 'plainbox/plainbox/impl/unit/testplan.py'
--- plainbox/plainbox/impl/unit/testplan.py	2015-04-14 12:17:11 +0000
+++ plainbox/plainbox/impl/unit/testplan.py	2015-06-12 15:06:42 +0000
@@ -159,6 +159,10 @@
         return self.get_record_value('exclude')
 
     @property
+    def required(self):
+        return self.get_record_value('required')
+
+    @property
     def icon(self):
         return self.get_record_value('icon')
 
@@ -202,13 +206,34 @@
 
         :returns:
             A CompositeQualifier corresponding to the contents of both
-            the include and exclude fields.
+            the include, exclude and required fields.
         """
         qual_list = []
         qual_list.extend(self._gen_qualifiers('include', self.include, True))
         qual_list.extend(self._gen_qualifiers('exclude', self.exclude, False))
+        qual_list.extend(self._gen_qualifiers('required', self.required, True))
         return CompositeQualifier(qual_list)
 
+    def get_required_jobs_qualifier(self):
+        """
+        Convert this test plan to qualifier that picks only required jobs.
+
+        :returns:
+            A FieldQualifier corresponding to the 'required' field.
+        """
+        field_qualifier_list = []
+        if self.required is not None:
+            field_origin = self.origin.just_line().with_offset(
+                self.field_offset_map['required'])
+            matchers_gen = self.parse_matchers(self.required)
+            for lineno_offset, matcher_field, matcher, error in matchers_gen:
+                if error is not None:
+                    raise error
+                offset = field_origin.with_offset(lineno_offset)
+                field_qualifier_list.append(FieldQualifier(
+                    matcher_field, matcher, offset, True))
+        return CompositeQualifier(field_qualifier_list)
+
     def _gen_qualifiers(self, field_name, field_value, inclusive):
         if field_value is not None:
             field_origin = self.origin.just_line().with_offset(
@@ -399,6 +424,7 @@
             description = 'description'
             include = 'include'
             exclude = 'exclude'
+            required = 'required'
             estimated_duration = 'estimated_duration'
             icon = 'icon'
             category_overrides = 'category-overrides'
@@ -434,6 +460,9 @@
             fields.exclude: [
                 NonEmptyPatternIntersectionValidator,
             ],
+            fields.required: [
+                NonEmptyPatternIntersectionValidator,
+            ],
             fields.estimated_duration: [
                 UntranslatableFieldValidator,
                 TemplateInvariantFieldValidator,
@@ -470,9 +499,9 @@
     methods on the test plan unit class itself can be implemented in an easier
     way.
 
-    The key data to handle are obviously the ``include`` and ``exclude``
-    fields. Those are used to come up with a qualifier object suitable for
-    selecting jobs.
+    The key data to handle are obviously the ``include``, ``exclude`` and
+    ``required`` fields. Those are used to come up with a qualifier object
+    suitable for selecting jobs.
 
     The second key piece of data is obtained from the ``include`` field and
     from the ``category-overrides`` and ``certification-status-overrides``
@@ -527,6 +556,8 @@
             self._get_qualifier_for(testplan, 'include', True))
         qual_list.extend(
             self._get_qualifier_for(testplan, 'exclude', False))
+        qual_list.extend(
+            self._get_qualifier_for(testplan, 'required', True))
         return CompositeQualifier(qual_list)
 
     def _get_qualifier_for(self, testplan, field_name, inclusive):