test: classify every skip in eight files as unproven or not-applicable - #623
test: classify every skip in eight files as unproven or not-applicable#623ian-flores wants to merge 2 commits into
Conversation
Second slice of #616. The unproven outcome shipped in #617 but nothing used it, so the distinction existed without changing what any real run reported. Triage eight files completely: 21 skips become attest.unproven, 32 become attest.not_applicable. The unproven ones share a shape -- VIP reached the thing it was asked to check and could not complete the check. An unreachable host in the TLS suite, an unreachable git repo, a version endpoint that returned no version, six Workbench UI controls that never appeared, a Kubernetes client that could not be built against a configured cluster, a session that never returned from resume. Each of those was previously indistinguishable from "this deployment does not have that feature". Add selftests/test_skip_triage.py, which fails when a bare pytest.skip appears in a file listed as triaged. Without it the next skip added to one of these files silently reintroduces the ambiguity, and the triage decays back to where it started.
There was a problem hiding this comment.
Pull request overview
This PR is the second slice of #616, converting bare pytest.skip() sites in eight product-test files into explicit vip.attest.not_applicable() vs vip.attest.unproven() classifications, and adding a selftest guard to prevent triaged files from regressing back to ambiguous skips.
Changes:
- Reclassified skip sites across eight
src/vip_tests/**modules intoattest.unproven()orattest.not_applicable()to distinguish “could not verify” vs “nothing to verify”. - Added
selftests/test_skip_triage.pyto enforce that triaged files contain no barepytest.skip()and still use theattesthelpers. - Updated
AGENTS.mdto document the new guard and expected skip-classification practice.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vip_tests/workbench/test_sessions.py | Converts a previously-plain skip to attest.unproven() for an incomplete suspend/resume verification. |
| src/vip_tests/workbench/test_session_capacity.py | Classifies capacity/profile-selection related skips as unproven vs not_applicable. |
| src/vip_tests/workbench/test_session_capacity_k8s.py | Classifies Kubernetes configuration/creation failures and profile-selection skips with attest. |
| src/vip_tests/workbench/test_jobs.py | Reclassifies UI-availability vs UI-failure skip paths using attest. |
| src/vip_tests/workbench/test_ide_launch.py | Switches internal “skip helper” calls to attest.not_applicable() and classifies missing kernel/output/console conditions as unproven. |
| src/vip_tests/prerequisites/test_versions.py | Converts configuration skips to not_applicable and missing-version-field skips to unproven. |
| src/vip_tests/cross_product/test_ssl.py | Reclassifies TLS/HTTPS preconditions as not_applicable and unreachable/runner-incapable checks as unproven. |
| src/vip_tests/connect/test_content_deploy.py | Marks missing runtimes as not_applicable, and unreachable external dependencies / missing content URL as unproven. |
| selftests/test_skip_triage.py | Adds a guard to prevent bare pytest.skip() from reappearing in triaged files. |
| AGENTS.md | Documents the new triage-guard expectation for contributors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except (PlaywrightTimeoutError, PlaywrightError): | ||
| pass | ||
| except (PlaywrightTimeoutError, PlaywrightError): | ||
| pass | ||
| pytest.skip(reason) | ||
| attest.not_applicable(reason) |
There was a problem hiding this comment.
Fixed in 4542bdc — the docstring now names attest.not_applicable.
| attest.unproven( | ||
| "RStudio Pro tab opened but Launch button did not appear — " | ||
| "the IDE may not be installed or fully available on this Workbench instance" | ||
| ) |
There was a problem hiding this comment.
Fixed in 4542bdc, and this one was mine to catch. The PR body argues this site is unproven precisely because the tab opening contradicts the "may not be installed" hedge — and then I left the hedge in the message. I fixed the classification and not the sentence that motivated it.
The reason now says the tab opened, its Launch button never appeared, no session could be started, and the Background Jobs checks below never ran, with an explicit note that the IDE is present and this is a UI or readiness problem.
| def test_triaged_file_actually_uses_the_helpers(relpath: str): | ||
| """Guards against 'triaging' a file by deleting its skips.""" | ||
| text = (_SRC / relpath).read_text() | ||
| assert "attest" in text, f"{relpath} is listed as triaged but never imports attest" |
There was a problem hiding this comment.
Agreed, fixed in 4542bdc. "attest" in text is satisfied by a comment or a docstring, so it pinned nothing. It now matches a real import (from vip import attest or import vip.attest) via imports_attest(), which has its own tests covering the comment and docstring cases that used to pass.
| _BARE_SKIP = re.compile(r"\bpytest\.skip\s*\(") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("relpath", TRIAGED_FILES) | ||
| def test_triaged_file_has_no_unclassified_skip(relpath: str): | ||
| path = _SRC / relpath | ||
| assert path.exists(), f"{relpath} moved or was deleted; update TRIAGED_FILES" | ||
| offenders = [ | ||
| f"{relpath}:{n}" | ||
| for n, line in enumerate(path.read_text().splitlines(), 1) | ||
| if _BARE_SKIP.search(line) | ||
| ] |
There was a problem hiding this comment.
Good catch, fixed in 4542bdc. This was the most useful of the four: the guard exists to stop the triage decaying, so being bypassable by a line break undercut the point of it.
It now scans the whole file text with \bpytest\s*\.\s*skip\s*\(, which tolerates whitespace anywhere the parser does, including a newline. I also extracted find_bare_skips() as a plain function so the guard's own behaviour is unit-tested rather than only asserted against the real tree — there are now tests for the split call, the spaced dot, multiple occurrences, and the request.node.skip(...) false positive.
One deliberate trade-off, documented in the docstring: the pattern also flags a pytest.skip written inside a comment or docstring. That false positive is loud and trivially reworded, whereas a missed real one is silent — and silence is the failure mode this guard exists to prevent.
|
Preview Links
|
Copilot review on #623, all four findings real. The guard scanned line by line for pytest.skip(, so the same call split across two lines -- or written with whitespace around the dot -- walked straight past it. A guard that a reformat can evade is not a guard. Scan the whole file with a whitespace-tolerant pattern instead, and extract find_bare_skips/imports_attest as plain functions so the guard's own behaviour is testable rather than asserted only against the real tree. The import check asserted the substring "attest" appeared somewhere in the file, which a comment or docstring satisfies. It now requires an actual import statement. Two source fixes. A helper docstring still promised pytest.skip after the call became attest.not_applicable. And the RStudio Pro job skip still read "the IDE may not be installed" on a branch only reachable after the tab opened and proved it is -- that contradiction is precisely why the site was reclassified as unproven, so leaving the wording was a half-fix.
Second slice of #616. Follows #617, which shipped the
unprovenoutcome but left it almost unused — the distinction existed without changing what any real run reported.What this does
Triages eight files completely: 21 skips become
attest.unproven, 32 becomeattest.not_applicable.The unproven ones all share a shape — VIP reached the thing it was asked to check and could not complete the check:
cross_product/test_ssl.pyconnect/test_content_deploy.pyprerequisites/test_versions.pyworkbench/test_jobs.pyworkbench/test_ide_launch.pyworkbench/test_session_capacity{,_k8s}.pyworkbench/test_sessions.pyEach was previously indistinguishable from "this deployment does not have that feature".
The guard
selftests/test_skip_triage.pyfails when a barepytest.skip()appears in a file listed as triaged. Without it the next skip added to one of these files silently reintroduces the ambiguity and the triage decays back to where it started. It also asserts a listed file actually importsattest, so a file cannot be "triaged" by deleting its skips.Confirmed non-vacuous: reintroducing a bare
pytest.skip()fails the guard and names the exact line.Judgement calls worth reviewing
workbench/test_auth.pyis deliberately untouched. It skips because the test only supports password auth — the same reasoning as thesso_onlyskip left alone in #617. #602's gap is a missing SSO-capable login test, not a misclassified skip; reclassifying it would be inconsistent and would not fix #602.Three left as not-applicable that reviewers may want to revisit, all previously decided in #268:
URL is not HTTPS,tls.insecure=true, andNo R versions available on Connect. Each is arguably a security or coverage check that did not happen, but #268 made a call and this PR does not quietly reverse it.Five sites were reclassified by hand after a keyword pass got them wrong. The clearest:
"RStudio Pro tab opened but Launch button did not appear"hedges that the IDE "may not be installed", but the tab opening proves it is — that skip was masking a UI regression as a feature-absence.Verification
ruff check/format --checkclean under CI's pinned 0.15.0;mypyclean on 35 files