refactor: fold duplicated _first_value into shared ssh_config - #787
Open
mrbobbytables wants to merge 2 commits into
Open
refactor: fold duplicated _first_value into shared ssh_config#787mrbobbytables wants to merge 2 commits into
mrbobbytables wants to merge 2 commits into
Conversation
tests/common/features/environment.py and tests/kde-smoke/features/environment.py each carried a byte-for-byte copy of the _first_value(*values) helper that already lives in tests/shared/ssh_config.py. Three copies of the same four-line function is exactly the copy-paste-over-shared-abstraction shape called out in projectbluefin#763: nothing enforced that a fix or behaviour change to one copy would land in the other two. Both suites now import _first_value from tests.shared.ssh_config instead of redefining it. No behaviour change — the three implementations were identical. Extend tests/unit/test_suite_environment_contract.py, which already parses every tests/*/features/environment.py with ast to enforce the quarantine-gate contract, with the same treatment for _first_value: a suite may not define its own copy, and any suite that calls it must import it from tests.shared.ssh_config. This closes the gap before it can reopen. Ref projectbluefin#763. Signed-off-by: mrbobbytables <mrbobbytables@users.noreply.github.com>
![sec-check[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Other unit tests can leave a minimal SSH module stub in sys.modules under xdist. Allow the probe patch to install run_ssh on that stub before exercising it. Assisted-by: Claude Sonnet 5 via GitHub Copilot CLI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sec-check[bot] <sec-check[bot]@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![kubestellar-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Approved by @castrojo for Hive auto-merge on green CI.
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
2 participants
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
Context: #763 flags that every behave suite hand-wires its own
environment.pywith copy-pasted cross-cutting concerns. One concretely named instance:_first_value(*values)is defined identically three times —tests/shared/ssh_config.py:32,tests/common/features/environment.py:23, andtests/kde-smoke/features/environment.py:35.What this does:
_first_valuedefinitions fromtests/commonandtests/kde-smoke; both now import the existing implementation fromtests.shared.ssh_config.tests/unit/test_suite_environment_contract.py(the ast-based contract test added by [architect] refactor: enforce behave suite environment quarantine-gate contract (new tests/unit/test_suite_environment_contract.py) #765 for the quarantine gate) with the same pattern for_first_value: a suiteenvironment.pymay not define its own copy, and if it calls the helper it must import it fromtests.shared.ssh_config. This turns the fix into an enforced contract instead of a one-off cleanup that could silently regress.Scope discipline (per #763's own recommendation-1 wording: "fold the duplicated
_first_valueinto the existingtests/shared/ssh_configdefinition"): this PR does only that. It deliberately does not touch:before_scenario/before_allpreamble into atests/shared/suite_env.py(proposed step 1 in [architect] behave suite environment contract is unenforced convention: 14 hand-wired environment.py, quarantine gate copy-pasted, timing wired in only 10/14 #763) — that is a 14-file, behaviour-sensitive refactor that needs a real E2E run behind it, already called out as future work by [architect] refactor: enforce behave suite environment quarantine-gate contract (new tests/unit/test_suite_environment_contract.py) #765.record_start/record_endtiming into the four suites that lack it — blocked on the still-open mandatory-vs-opt-in decision in [architect] tests/shared/timing.py is an orphaned subsystem: timings.jsonl has no consumer, DEFAULT_SLA and SLA_STRICT are never read outside their own tests #764.Verification:
pytest tests/unit -q— 1541 passed.ruff checkon the two editedenvironment.pyfiles shows the same 4 pre-existing warnings as before the change (verified viagit stash) — no new lint issues introduced.Ref #763.
— hive: backend=copilot model=claude-sonnet-5
🐝 Hive Agent:
contributor| SHA:8f272f09