Ensure that jobs that have an after suspend counterpart are run before suspend (New) #1037
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
==========================================
+ Coverage 40.67% 40.76% +0.08%
==========================================
Files 335 336 +1
Lines 37423 37460 +37
Branches 6359 6370 +11
==========================================
+ Hits 15222 15269 +47
+ Misses 21560 21550 -10
Partials 641 641
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
kissiel
suggested changes
Mar 5, 2024
…e suspend
Some jobs should be run before *and* after suspend. To take this into
account, the `siblings` field was developed, along with the
`also-after-suspend` and `also-after-suspend-manual` flags[2].
These would let Checkbox spawn a similar job with an added dependency on
the suspend job (either the manual or the automated version of it).
The problem is that the original job did not have any dependency to
force it to be run *before* the suspend job.
This was not an issue for test plans organized manually, using regular
expressions, because you could have an include section that looks like:
storage_.*
com.canonical.certification::suspend/suspend_advanced_auto
after-suspend-storage.*
However, now that template ids can be added in test plans[3], this is a
problem.
This patch will make sure the jobs that need to run before suspend are
added as dependencies of their related suspend job, so that regardless
of the order in the test plan, they will be run in the proper order.
Fix: #1010
[1]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#job-siblings-field
[2]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#also-after-suspend-flag
[3]
https://checkbox.readthedocs.io/en/latest/reference/units/test-plan.html
_get_suspend_inhibitor_list() and _get_before_suspend_dependency_set() work very similarly. This patch introduces a new helper function, _is_job_impacting_suspend(), which helps identifying jobs that should run before the manual and automated suspend jobs. Both _get_suspend_inhibitor_list() and _get_before_suspend_dependency_set() make use of it to prevent code duplication.
With the new helper function, less tests are required.
If the filtering is done on the whole job_list, the suspend job will
depend on jobs that are not required (undesired) for the current session
run.
As a result, the suspend job is skipped with reasons like:
required dependency 'com.canonical.certification::audio/
playback_auto' did not run yet
and all of the after-suspend jobs are skipped!
the job_list therefore needs to be narrowed down to remove the
"undesired" jobs and only keep the ones that are actually going to run.
pieqq
force-pushed
the
1010-suspend-job-update
branch
from
d0e463a
to
f33a07b
Compare
LiaoU3
pushed a commit
to LiaoU3/checkbox
that referenced
this pull request
Mar 20, 2024
…ore suspend (New) (canonical#1037) Some jobs should be run before *and* after suspend. To take this into account, the `siblings` field was developed[1], along with the `also-after-suspend` and `also-after-suspend-manual` flags[2]. These would let Checkbox spawn a similar job with an added dependency on the suspend job (either the manual or the automated version of it). The problem is that the original job did not have any dependency to force it to be run *before* the suspend job. This was not an issue for test plans organized manually, using regular expressions, because you could have an include section that looks like: storage_.* com.canonical.certification::suspend/suspend_advanced_auto after-suspend-storage.* However, now that template ids can be added in test plans[3], this is a problem. This patch will make sure the jobs that need to run before suspend are added as dependencies of their related suspend job, so that regardless of the order in the test plan, they will be run in the proper order. Documentation is updated to mention guarantee of job running before suspend in Job Unit reference page. Fix: canonical#1010 [1] https://checkbox.readthedocs.io/en/stable/reference/units/job.html#job-siblings-field [2] https://checkbox.readthedocs.io/en/stable/reference/units/job.html#also-after-suspend-flag [3] https://checkbox.readthedocs.io/en/latest/reference/units/test-plan.html
binli
pushed a commit
to binli/checkbox
that referenced
this pull request
Mar 22, 2024
…ore suspend (New) (canonical#1037) Some jobs should be run before *and* after suspend. To take this into account, the `siblings` field was developed[1], along with the `also-after-suspend` and `also-after-suspend-manual` flags[2]. These would let Checkbox spawn a similar job with an added dependency on the suspend job (either the manual or the automated version of it). The problem is that the original job did not have any dependency to force it to be run *before* the suspend job. This was not an issue for test plans organized manually, using regular expressions, because you could have an include section that looks like: storage_.* com.canonical.certification::suspend/suspend_advanced_auto after-suspend-storage.* However, now that template ids can be added in test plans[3], this is a problem. This patch will make sure the jobs that need to run before suspend are added as dependencies of their related suspend job, so that regardless of the order in the test plan, they will be run in the proper order. Documentation is updated to mention guarantee of job running before suspend in Job Unit reference page. Fix: canonical#1010 [1] https://checkbox.readthedocs.io/en/stable/reference/units/job.html#job-siblings-field [2] https://checkbox.readthedocs.io/en/stable/reference/units/job.html#also-after-suspend-flag [3] https://checkbox.readthedocs.io/en/latest/reference/units/test-plan.html
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Some jobs should be run before and after suspend. To take this into
account, the
siblingsfield was developed, along with thealso-after-suspendandalso-after-suspend-manualflags.These would let Checkbox spawn a similar job with an added dependency on
the suspend job (either the manual or the automated version of it).
The problem is that the original job did not have any dependency to
force it to be run before the suspend job.
This was not an issue for test plans organized manually, using regular
expressions, because you could have an include section that looks like:
However, now that template ids can be added in test plans, this is a
problem.
This patch will make sure the jobs that need to run before suspend are
added as dependencies of their related suspend job, so that regardless
of the order in the test plan, they will be run in the proper order.
Resolved issues
https://warthogs.atlassian.net/browse/CHECKBOX-1265
Documentation
The Job Unit reference page has been updated to explain what happens when the
also-after-suspendflag is used, or when asiblingis defined that depends on a suspend job.Tests
Given the following units:
Before applying the patch:
The
demo-test-storage-sdbjob should be run beforesuspend/suspend_advanced_auto, but is run after.After applying the patch:
I also ran the test plan to make sure it executed properly and in the right order.