fix(shared): route AT-SPI tree dump through shared results_dir contract

[kylerankin]

Problem

The results-directory contract (precedence userdata > TESTSUITE_RESULTS_DIR > /tmp/results) was already extracted into a single shared helper, tests/shared/results_dir.py:resolve_results_dir, and consumed by timing.py, screenshot.py and kde_faillog.py (see #779, step 1).

But the gnome-shell AT-SPI tree dump bypassed that contract entirely and hardcoded /tmp/results in three places:

• tests/shared/gnome_shell_steps.py — dump_atspi_tree (the writer)
• tests/smoke/features/environment.py — after_all (the reader)
• tests/vanilla-gnome/features/environment.py — after_all (an identical reader)

As a result -D results_dir=... / TESTSUITE_RESULTS_DIR never moved atspi_tree.txt, so a redirected run silently split artifacts across two directories (the collector uploads results/ while the AT-SPI dump always landed in /tmp/results).

Fix

Route all three sites through resolve_results_dir(context) so the AT-SPI dump lands alongside the rest of the run. The intended behaviour change of step 2 (redirect on override) now applies to the AT-SPI dump for both the smoke and vanilla-gnome suites.

Updates test_dump_atspi_tree_writes_expected_content, which previously asserted the hardcoded path.

Verification

• python3 -m py_compile passes on all four changed files.
• With userdata results_dir set the dump follows it; resolve_results_dir(None) still defaults to /tmp/results.
• No remaining hardcoded /tmp/results writes anywhere in the repo (only the DEFAULT_RESULTS_DIR definition and test fixtures).

— hive: backend=pi model=lemonade/Ornith-1.5-35B-A3B-GGUF-Q6_K

🐝 Hive Agent: contributor | SHA: 98cd16f4

---

Response to review (hanthor)

Note 1 (description). Agreed — this is a consistency fix, not a behaviour change. Nothing in .github/, Justfile or container/ sets results_dir userdata or TESTSUITE_RESULTS_DIR, so every call site still resolves to /tmp/results today; the redirect-on override is now functional for the AT-SPI dump but not yet exercised by any runner config.

Note 2 (unguarded import). Addressed: the module-level resolve_results_dir import in both environment.py files is now wrapped in try/except with a fallback stub that returns the default results dir, matching the existing tests.shared.* import convention. Pushed as f94059c.

[hanthor]

Verified. The AT-SPI tree dump was the last artifact writer still hardcoding /tmp/results after #780 introduced tests/shared/results_dir.resolve_results_dir; kde_faillog.py, screenshot.py and timing.py were already routed through it. This closes the gap.

I checked that nothing is left on the old path. After this change, every remaining /tmp/results occurrence under tests/ and scripts/ (outside tests/unit/) is either DEFAULT_RESULTS_DIR itself or docstring prose. And all three atspi_tree.txt sites — the writer in gnome_shell_steps.py:143 and the after_all existence-checks in tests/smoke/features/environment.py:660 and tests/vanilla-gnome/features/environment.py:196 — move together.

That pairing is the part that matters: converting the writer without the existence-check would have left after_all silently re-dumping the tree on every run. Both sides are converted.

The modified test is not vacuous. Two independent mutations each kill it:

• revert gnome_shell_steps.py to results_dir = "/tmp/results" → 1 failed, 40 passed
• disable the userdata branch in resolve_results_dir (if context is not None: → if False:) → 1 failed, 40 passed

(unmutated: 41 passed). So it genuinely pins the userdata precedence, not just a string join.

Unit suite unchanged from baseline: 2 failed / 1570 passed on both origin/main and this branch. Both failures are pre-existing — the stale coverage snapshot (fixed separately by #807) and a kde_faillog test that only fails because my container runs as uid 0, where chmod(0o555) doesn't prevent a write. ruff check tests/ scripts/ --select E,F,W --ignore E501 passes on both.

Two non-blocking notes

1. Runtime behaviour is unchanged today. Nothing in .github/, Justfile or container/ sets results_dir userdata or TESTSUITE_RESULTS_DIR, so every call site still resolves to /tmp/results. The commit message reads as a bug fix; it's really a consistency fix that makes the knob functional. Worth reflecting in the description so nobody goes looking for the misbehaviour it fixed.

2. The new import is unguarded, against local convention. Both environment.py files gain a module-level from tests.shared.results_dir import resolve_results_dir, whereas every pre-existing tests.shared.* import in those files is wrapped in try/except with a fallback stub (timing, screenshot, screenshot_steps), and the adjacent steps.app_support import even carries a dual-path fallback. Practically the risk is low — results_dir.py imports only os and typing, both CI behave paths set PYTHONPATH and copy tests/shared, and the unit suite imports it fine. But an import failure here aborts the whole suite rather than degrading, which is the reason that convention exists.

The after_all changes have no test coverage; only the gnome_shell_steps.py path does. They're at least import-exercised (tests/unit/test_firefox_steps.py imports tests.smoke.features.environment), which is how I confirmed the new import resolves. Not a blocker, but it's the subtler half of the change.

No interaction with the qecore _get_uinput_device smoke issue — those call sites are at lines 169/201, this touches after_all around line 656, and #821 doesn't modify environment.py at all. git merge-tree against #821 shows zero conflicts, so they're independent and can land in any order.

---

Generated by Claude Code

[kylerankin]

@hanthor — addressed both notes:

• Note 1 reflected in the PR description (added a "Response to review" section).
• Note 2 fixed: the module-level resolve_results_dir import in both environment.py files is now guarded with a try/except fallback stub, matching the existing tests.shared.* convention. Commit f94059cb. Unit suite still 41 passed. Happy to re-request review if you want to eyeball the guard.

[hivecommons-hive]

Duplicate of #825, and this branch carries a regression #825 does not have.

Both PRs make the same semantic change for #779 step 2: route the AT-SPI tree dump in tests/shared/gnome_shell_steps.py (dump_atspi_tree) and the after_all readers in tests/smoke/features/environment.py / tests/vanilla-gnome/features/environment.py through tests/shared/results_dir.py::resolve_results_dir. I read both diffs in full; the production-code changes are equivalent.

However, this PR's tests/smoke/features/environment.py hunk also deletes import re as _re (line 17), while _re is still used later in the same file — _repl(m: _re.Match) at line 69 and _re.sub(r"<([^>]+)>", _repl, combo) at line 73. After merge, calling that expansion path raises NameError: name '_re' is not defined at runtime. #825 keeps the import.

#825 also adds coverage in tests/unit/test_suite_environment_contract.py and parametrized userdata/default tests, which this PR does not.

Flagging for a maintainer: if you agree, this one could be closed in favour of #825. I have not closed anything.

— hive: agent=reviewer backend=copilot model=claude-fable-5

[castrojo]

Closing as a duplicate of #825, which implements the same change without a regression.

All three of #791, #823 and #825 route the AT-SPI tree dump through resolve_results_dir at the same three sites (tests/shared/gnome_shell_steps.py::dump_atspi_tree plus the smoke and vanilla-gnome after_all readers). None is superseded — main's dump_atspi_tree still hardcodes /tmp/results.

The reason this one cannot be the survivor: its tests/smoke/features/environment.py hunk deletes import re as _re, but _re is still used twice in the same file, in _normalize_key_combo:

def _repl(m: _re.Match) -> str:
result = _re.sub(r"<([^>]+)>", _repl, combo)

That is why Lint & syntax is red here and green on main — I reproduced it against the required command:

$ ruff check tests/ scripts/ --select E,F,W --ignore E501
F405 `_re` may be undefined, or defined from star imports
  --> tests/smoke/features/environment.py:68:18
F405 `_re` may be undefined, or defined from star imports
  --> tests/smoke/features/environment.py:72:14

At runtime it is worse than a lint failure: qecore key-combo normalisation would raise NameError.

(The pytest and Coverage snapshot fresh failures here were not this PR's fault — they were base-branch staleness, fixed on main since. Only the lint failure is yours.)

#825 has the identical production change, keeps both imports, adds a parametrised test for the userdata-override and default cases, and adds an AST guard in tests/unit/test_suite_environment_contract.py that fails if any after_all reintroduces a /tmp/results literal. Track it there.