tests: run dispvm preload tests once instead of per-template#785
Conversation
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
2 times, most recently
from
42011c1 to
61a528f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #785 +/- ##
==========================================
- Coverage 70.15% 70.11% -0.04%
==========================================
Files 61 61
Lines 13995 13997 +2
==========================================
- Hits 9818 9814 -4
- Misses 4177 4183 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Please read again the issue. It is not all preload tests. I listed on the first the specific issues that need to be extracted to a separate class. |
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
from
61a528f to
af49ae6
Compare
ben-grande
left a comment
ben-grande
left a comment
There was a problem hiding this comment.
Just to let you know, you are in the right track.
For reviewers, split view seems better than unified view in this instance.
| self.app.remove_handler, "domain-add", self._on_domain_add | ||
| ) | ||
| self.adminvm = self.app.domains["dom0"] | ||
| self.init_default_template(self.template) |
There was a problem hiding this comment.
Why? This is making more changes than what the commit should be, solely related to having a reusable class and a separate class for preloaded disposable tests that runs on all templates from the ones that don't need to run on all templates, just one. I think it makes sense to have it in the helper class also.
| "--", | ||
| "qubesdb-read /name | tr -d '\n'", | ||
| ] | ||
| logger.info("end") |
There was a problem hiding this comment.
|
|
||
| def tearDown(self): # pylint: disable=invalid-name | ||
| logger.info("start") | ||
| if "gui" in self.disp_base.features: |
There was a problem hiding this comment.
| ) | ||
| except subprocess.CalledProcessError as e: | ||
| stdout = getattr(e, "stdout", str(e)) | ||
| logger.critical("{}: {}: {}".format(qube.name, cmd, stdout)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
This and other things haven't been addressed. Please check all comments.
| self.app.domains["dom0"], | ||
| default_dispvm, | ||
| self.disp_base, | ||
| self.disp_base_alt, |
There was a problem hiding this comment.
|
|
||
| def load_tests(loader, tests, _pattern): | ||
| tests.addTests(loader.loadTestsFromNames(create_testcases_for_templates())) | ||
| tests.addTests(loader.loadTestsFromTestCase(TC_21_DispVM_Preload)) |
There was a problem hiding this comment.
I think this won't run on any template. I think you should add some option to qubes.tests.create_testcases_for_template() to run the test on the default template. You might need to call init_default_template() from there.
| if default_template_only and templates: | ||
| # Some tests are template-agnostic and should run only once | ||
| # to avoid slowing down the integration test suite. | ||
| templates = templates[:1] |
There was a problem hiding this comment.
This doesn't get the default template. But in case someone set QUBES_TEST_TEMPLATES for openqa, it may not match the app.default_template. Get the first template in that list if the variable is set, else, use self.app.default_template (self is not passed to that function, so it needs to be passed another way).
| super().setUp() | ||
| self.setup_dispvm_nodes() | ||
| logger.info("end") | ||
| See QubesOS/qubes-issues#10749 |
There was a problem hiding this comment.
I don't think there is need to have this comment. If you git blame the PR, you will see this issue in the commit message and in the PR message.
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
from
e79bd1a to
4e27c1f
Compare
| else: | ||
| try: | ||
| app = qubes.Qubes() | ||
| if app.default_template: |
There was a problem hiding this comment.
Marek, do you have a recommendation to avoid instantiating Qubes() here?
There was a problem hiding this comment.
While I don't have an idea for avoiding creating Qubes() instance to list templates (in absence of QUBES_TEST_TEMPLATES variable), I don't think you need to do that at all. Tests that are not template-specific (that would run on all (selected) templates) simply use default template, and have no template name in the test instance name. There is absolutely no need to handle the case of default template not being set.
I'd argue even the above choosing first template from QUBES_TEST_TEMPLATES (instead of the default one) is not necessary.
There was a problem hiding this comment.
@therohityadav I am sorry, it seems that I created this confusion (#785 (comment)). The old code seems correct.
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
2 times, most recently
from
60c3370 to
facc9ee
Compare
|
Reviewing on github is kinda hard because it shows "added" and "removed" lines, but not "moved" lines, which would facilitate a lot. I checked the first commit on a disposable and it seems the changes are fine, except that commit must be squashed first (a single commit). |
Use default_template_only to avoid running the preload test for every template. This significantly reduces CI runtime while preserving test coverage. Fixes QubesOS/qubes-issues#10749
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
from
facc9ee to
2faea4b
Compare
Thank you for testing it locally, glad it works! I’ve squashed the commits into a single commit as requested. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032922-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032404-devel&flavor=update
Failed tests26 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 28 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:13 performance degradations
Remaining performance tests:91 tests
|
I haven't tested it locally. When I said that "I checked the first commit", I meant that I did "git diff". Not that I "checked out the commit and ran the tests". So know I see how what I wrote could be misinterpreted. But the context from the first phrase is important:
I'd like you to look at the openqa run and analyze it and report your findings. I want you to understand what changed on openqa. Tip: compare with previous run. I will start: compared to a previous run from 3 days ago in "pull requests" group, it shortened the total run time in 50 minutes. I intentionally didn't link the previous run so you search for it. |
|
|
||
|
|
||
| def create_preload_testcases_for_default_template(): | ||
| return qubes.tests.create_testcases_for_templates( |
There was a problem hiding this comment.
Tests that are supposed to be running just on one template don't need the mixin approach at all. Simply define class inheriting from qubes.tests.SystemTestCase directly and call self.init_default_template() in setUp(). To make the changes simpler, you can also add self.template = self.app.default_template, so you still have the self.template attribute like in the other tests.
And then there is no need calling create_testcases_for_templates() for those tests at all, as unittest/pytest/nose will already correctly discover them automatically.
This is an example of test class running on a single (default) template: https://github.com/QubesOS/qubes-core-admin/blob/main/qubes/tests/integ/basic.py#L52
|
Hey @marmarek, the latest commit triggered a CI failure, but it seems to happen at the very end during the artifacts upload step: FATAL: flag provided but not defined: -timeout From the logs, this appears to be related to a GitLab Runner mismatch (runner 18.10.0 vs helper 18.7.2), rather than a failure in the test execution itself. Could you please confirm if this is an infrastructure issue, or if I should recheck anything on my side? |
Yup, that's unrelated to your PR. |
Those failures look to be related to this PR. |
Inherits directly from SystemTestCase to bypass the template factory and reverts __init__.py changes to the default original file. Assigns self.template to .name to prevent TypeErrors in helper methods.
therohityadav
force-pushed
the
fix-10749-repeated-dispvm-tests
branch
from
51512c4 to
f28f2dc
Compare
|
The split between the two commits is not great, but we can live with that. |
4 participants
This patch isolates template-agnostic DispVM preload tests so they run only once instead of once per template.
DispVMHelpersMixin: Moves shared setup, teardown, and helper logic into a base mixin without template inference.TC_20_DispVMMixin: Keeps the OS/GUI-dependent tests that run for each template.TC_21_DispVM_Preload: Adds a standalone class to run template-independent preload tests once.The following tests are moved to run only once:
test_011_preload_reject_maxtest_012_preload_low_memtest_015_preload_race_moretest_016_preload_race_lesstest_017_preload_autostarttest_018_preload_globaltest_019_preload_discard_outdated_volumestest_020_preload_discard_outdated_volume_sizetest_021_preload_discard_outdated_settingFixes QubesOS/qubes-issues#10749