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

Proposed by Maciej Kisielewski
Status: Work in progress
Proposed branch: lp://qastaging/~kissiel/checkbox/fix-1391774-required-field-in-tp
Merge into: lp://qastaging/checkbox
Diff against target: 206 lines (+82/-7)
4 files modified
checkbox-ng/checkbox_ng/commands/newcli.py (+5/-1)
plainbox/docs/manpages/plainbox-test-plan-units.rst (+9/-0)
plainbox/plainbox/impl/unit/test_testplan.py (+33/-2)
plainbox/plainbox/impl/unit/testplan.py (+35/-4)
To merge this branch: bzr merge lp://qastaging/~kissiel/checkbox/fix-1391774-required-field-in-tp
Reviewer Review Type Date Requested Status
Checkbox Developers Pending
Review via email: mp+261860@code.qastaging.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

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

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

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

3830. By Maciej Kisielewski

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

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

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>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches