[architect] refactor: single source of truth for the results-directory contract — new tests/shared/results_dir.py; timing.py/screenshot.py/kde_faillog.py import it - #780
Open
kubestellar-hive[bot] wants to merge 1 commit into
Conversation
…y contract tests/shared/timing.py, tests/shared/screenshot.py and tests/shared/kde_faillog.py each carried a byte-identical private _results_dir() implementing the same precedence rule (behave userdata results_dir > TESTSUITE_RESULTS_DIR > /tmp/results), including the same docstring. Three copies means a change to the contract has to be made three times, and only the kde_faillog copy had a direct unit test. Extract the rule into tests/shared/results_dir.resolve_results_dir() and have the three sites import it under their existing private name, so no call site or test reference changes. Pure de-duplication: the resolution order, the env var name and the /tmp/results default are unchanged. The fourth site, gnome_shell_steps.dump_atspi_tree, hardcodes /tmp/results and does not honour the contract at all; converting it changes where an artifact lands when results_dir is overridden, so it is left for a separate change and recorded in the issue. Refs #779 Signed-off-by: sec-check[bot] <sec-check[bot]@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or 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
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.
Refactor
Cluster claimed: the results-directory resolution contract in
tests/shared.Files touched:
tests/shared/results_dir.py(new),tests/shared/timing.py,tests/shared/screenshot.py,tests/shared/kde_faillog.py.Function extracted: the three byte-identical private
_results_dir()definitions(
timing.py:9,screenshot.py:18,kde_faillog.py:108) become onetests/shared/results_dir.resolve_results_dir().What changes structurally
Every artifact writer in
tests/sharedresolves its output directory with the sameprecedence — behave userdata
results_dir> envTESTSUITE_RESULTS_DIR>/tmp/results.That rule was stated three times, byte-for-byte (same body, same docstring), with no
shared definition. Changing the precedence meant editing three copies, and only the
kde_faillogcopy was directly unit-tested(
tests/unit/test_kde_faillog.py:320), so a divergence introduced intiming.pyorscreenshot.pywould not have been caught.This adds
tests/shared/results_dir.pyexportingresolve_results_dir(context=None)andDEFAULT_RESULTS_DIR, and rebinds the three sites to it:Importing under the existing private name means no call site and no test reference
changes —
tests/unit/test_kde_faillog.pystill exercises the contract throughmod._results_dir, now against the single shared implementation.What does NOT change
Pure de-duplication. The resolution order, the userdata key, the env var name and the
/tmp/resultsdefault are all identical. No behaviour change, no feature addition.Deliberately out of scope
tests/shared/gnome_shell_steps.py:143-144hardcodes/tmp/resultsforatspi_tree.txtand does not honour the contract at all (its reader,
tests/smoke/features/environment.py:457, hardcodes the same path independently).Converting those two sites changes where an artifact lands when
results_dirisoverridden, so it is not bundled into a no-behaviour-change refactor. It is recorded as
step 2 in the issue.
Disjointness
No open testsuite PR touches these files. Checked against the current open set:
#776 (
tests/unit/test_update_coverage_snapshot.py), #774 (tests/unit/test_compose_e2e_image.py),#757 (
tests/smoke/features/steps/system_health_steps.py,tests/unit/test_system_health_steps.py),#751 (
tests/unit/test_screenshot_steps.py— the behave wrappers inscreenshot_steps.py,not
screenshot.py). This PR adds no file undertests/unit/.Validation
ruff check tests/ scripts/ --select E,F,W --ignore E501— All checks passed (matches thepr-validate.ymllint gate).pytest tests/unit— 1278 passed (test_validate_docs.pyexcluded: it fails to import onthis machine for an unrelated pre-existing reason,
scripts/validate_docs.pyraisingSystemExit(1)at import).python3 scripts/update_coverage_snapshot.py --check— OK, snapshot still up to date.Refs #779
Filed by architect agent (ACMM L5 — hold-gated mode). Hold-gated: human review required. Do not merge without human approval.
— hive: agent=architect backend=copilot model=claude-opus-5