feat(reporting): report checks VIP could not verify as unproven - #617
Draft
ian-flores wants to merge 1 commit into
Draft
feat(reporting): report checks VIP could not verify as unproven#617ian-flores wants to merge 1 commit into
ian-flores wants to merge 1 commit into
Conversation
An ordinary skip and a check VIP was asked to run but could not were reported identically and neither affected the exit code, so a run in which a configured product went entirely unverified exited 0 and rendered a report with no failures. To anyone reading that report as an audit artifact, "we did not check" and "we checked and it is fine" looked the same. Add vip.attest, whose two helpers make a skip site say which it means: not_applicable() for "nothing to verify here", unproven() for "asked to verify this and could not". An unproven result gets its own report status and badge, an UNPROVEN: prefix in JUnit, SARIF level warning, and exit code 6 -- distinct from pytest's 1 so a pipeline can tell a broken deployment from an unchecked one. --allow-unproven restores the previous behaviour. A bare pytest.skip() still means not_applicable, so the existing skip sites keep their meaning until each is triaged deliberately. The two Workbench auth-gate skips are converted here: a configured Workbench whose authentication never completed is exactly the case where every test falls away and the run reports success. Closes #596
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit “unproven” outcome to distinguish checks that VIP could not verify from ordinary skips that are not applicable, and propagates that distinction through reporting outputs and the process exit code (exit 6 unless --allow-unproven is used). This directly addresses the failure mode in #596/#616 where a configured product can silently go unverified while the run exits 0.
Changes:
- Add
vip.attest(not_applicable/unproven) and carry anunprovenflag throughresults.json, HTML rendering, JUnit, and SARIF. - Update the pytest plugin to detect unproven skips via a sentinel, emit an “unproven” exit status, and add
--vip-allow-unproven; addvip verify --allow-unprovenand forward it to pytest. - Convert Workbench auth-gate skips to
attest.unprovenand add/extend selftests + docs to lock in the new behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vip/attest.py | New API for classifying skips as not-applicable vs unproven (sentinel-based transport). |
| src/vip/plugin.py | Detect unproven skips, record unproven in results, add allow flag, and promote exit status to 6. |
| src/vip/reporting.py | Add unproven to data model; emit UNPROVEN-prefixed messages in JUnit/SARIF and SARIF warning level. |
| src/vip/report_html.py | Add UNPROVEN badge/style, grouping/labels, and explanatory copy in skip cards. |
| src/vip/cli.py | Add vip verify --allow-unproven and forward to pytest plugin option. |
| src/vip_tests/workbench/conftest.py | Switch Workbench auth-failure skips to attest.unproven via a helper. |
| selftests/test_workbench_skip.py | Assert Workbench auth skip is flagged unproven and survives classification. |
| selftests/test_reporting.py | Add coverage for TestResult.status == unproven, counts, and machine-format behavior. |
| selftests/test_report_html.py | Verify HTML grouping, labels, badge style, and explanation for unproven. |
| selftests/test_plugin.py | Verify sentinel stripping, unproven flagging, and exit-code contract (incl. allow flag). |
| selftests/test_cli_verify.py | Verify CLI flag parsing and forwarding into pytest invocation. |
| README.md | Document unproven semantics in JUnit/SARIF and exit code 6 + --allow-unproven. |
| docs/test-architecture.md | Update guidance to distinguish skip vs unproven and describe pipeline propagation. |
| AGENTS.md | Update contributor guidance to prefer attest.* helpers over bare pytest.skip(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
1234
to
+1235
| elif report.outcome == "skipped": | ||
| skip_reason = _extract_skip_reason(report.longrepr) | ||
| skip_reason, unproven = _classify_skip_reason(_extract_skip_reason(report.longrepr)) |
| print( | ||
| f"\nVIP: {len(unproven)} check(s) could not be verified. Nothing failed, " | ||
| f"but nothing was proven either:\n{reasons}\n" | ||
| "Pass --allow-unproven to treat these as an ordinary skip.", |
Contributor
|
Preview Links
|
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.
Closes #596. First slice of #616.
The problem
An ordinary skip and a check VIP was asked to run but could not were reported identically, and neither affected the exit code. A run in which a configured product went entirely unverified exited 0 and rendered a report with no failures. To anyone reading that report as an audit artifact, "we did not check" and "we checked and it is fine" looked the same.
#596 has the worked example: Workbench configured via
--workbench-url, authentication failed, every Workbench test skipped, exit code 0.What this does
Adds
vip.attest, whose two helpers make a skip site say which kind of skip it is:attest.not_applicable(reason)-- there was nothing to verify here (product not configured, tier lacks the feature). The run is still complete.attest.unproven(reason)-- VIP was asked to verify something and could not. Nothing failed, but nothing was proven either.An unproven result carries through the whole pipeline: its own
UNPROVENbadge and explanatory card in the HTML report, anUNPROVEN:prefix on the JUnit<skipped>message, SARIF levelwarning, and exit code 6 from the run. Six rather than one so a pipeline can tell "the deployment is broken" from "the deployment could not be checked".--allow-unprovenrestores the previous behaviour.The classification travels from the skip site to the report as a sentinel prefix on the skip reason, because the reason string is the one part of a skip pytest carries intact across process boundaries -- it works from a fixture, from a step definition, and under xdist, none of which reliably reach the item stash. The sentinel is stripped before anything human-facing sees it.
Behaviour change
vip verifynow exits 6 instead of 0 when a check goes unproven. Lanes that were quietly skipping will start failing. That is the intended effect, but it is a real change on upgrade and--allow-unprovenexists for anyone who needs the old contract while they triage.A deliberate departure from #616 as filed
#616 proposes that any skip is unproven by default, with sites opting out. This PR inverts that: a bare
pytest.skip()still means not-applicable, and sites are converted deliberately.Default-unproven would make "product not configured" fatal, and that is by far the most common skip in the suite -- #596's own note on the reverse risk calls out that exactly this case must stay non-fatal. So the burden shifts by triage rather than by flag day. Retriaging the remaining skip sites is the follow-up #616's scope note already describes.
The two Workbench auth-gate skips are converted here, since a configured Workbench whose authentication never completed is precisely the case where every test falls away and the run reports success. The
sso_onlyskip beside them is deliberately left as an ordinary skip: an SSO deployment genuinely has no password form to exercise, so flagging it would fail every SSO deployment's own verification run.Verification
ruff checkandruff format --checkclean under CI's pinned 0.15.0;mypyclean; product tests still collect (139)--allow-unproven, and the sentinel absent fromresults.json,junit.xmlandresults.sarifwhile each still names the classification