Skip to content

ci: de-duplicate invariant gates across the test matrix - #372

Open
codeforester wants to merge 1 commit into
mainfrom
ci/312-20260918-deduplicate-invariant-gates
Open

codeforester wants to merge 1 commit into
mainfrom
ci/312-20260918-deduplicate-invariant-gates

Conversation

@codeforester

Copy link
Copy Markdown
Contributor

Fixes #312

Summary

  • Keep the complete OS/Python runtime suite in the compatibility matrix, while running coverage, typing, lint, docs/contracts, benchmark, and security gates once in the quality job.
  • Split tests/full_validate.sh into named, reusable gate groups while preserving its no-argument local aggregate and result-file behavior.
  • Limit Tests workflow push runs to main and version tags; feature branches are validated by their pull-request run instead of a duplicate push run.
  • Keep Package workflow focused on build, artifact, metadata, and installed-wheel release-boundary checks.

Validation

  • ./tests/full_validate.sh: 526 passed, 193 subtests passed, 87.20% total coverage; typing, Ruff, docs/schema/contracts, benchmark budget, Bandit, and pip-audit all passed.
  • ./tests/full_validate.sh --gate runtime: 526 passed, 193 subtests passed.
  • Both changed workflow files parse as YAML; git diff --check passed.

Hosted timing baseline

Before-change successful Tests runs on main (four runs, each 19 jobs) had median total job time of 29.3 minutes and median workflow wall time of 7.1 minutes. I will update this PR with the corresponding post-change hosted measurements once its jobs complete. No PRs in this train are being merged.

Comment thread tests/full_validate.sh
# unpublished editable checkout itself.
local audit_requirements
audit_requirements="$(mktemp)"
trap 'rm -f "$audit_requirements"' RETURN

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness: cleanup trap no longer fires on the failure path it exists for

run_security() replaced the old script-wide trap ... EXIT with a function-scoped trap 'rm -f "$audit_requirements"' RETURN. Under set -e (active at the top of this script), a RETURN trap only fires when the function returns normally — if a command inside the function fails and triggers errexit, the whole script exits immediately without unwinding through the function's return, so the RETURN trap never runs.

Confirmed empirically (bash 5.3):

f() { local tmp; tmp="$(mktemp)"; trap 'rm -f "$tmp"' RETURN; false; }
case sec in sec) f ;; esac   # under set -e

→ the trap never fires and the temp file is leaked; the equivalent test with trap ... EXIT at script scope does clean up.

Failure scenario: pip-audit --strict (line 90) finds a vulnerability and exits non-zero — exactly the case this gate exists to catch. The mktemp file created at line 85 is now left behind in $TMPDIR instead of being removed, unlike the pre-refactor script. Restoring an EXIT trap (or an explicit cleanup on both paths) would fix it.

Comment thread tests/full_validate.sh
if ((node_contracts_missing)); then
printf '%s\n' '{"status":"partial","skipped":["node contract validator"]}' > "$validation_result"
printf 'Validation result: partial; Node.js contract validation was skipped (%s).\n' "$validation_result"
return 2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed behavior: dropped user-facing guidance on the partial-result path

The pre-refactor script's node-missing branch printed three lines, the last of which was:
printf 'This result is non-authoritative; install Node.js for the full gate.\n'

write_validation_result() keeps the JSON-status line and the 'Validation result: partial…' line but drops this one. A developer running ./tests/full_validate.sh (no args) on a machine without Node.js now sees the partial-result line and a bare exit 2, without the explicit remediation hint that used to accompany it. Low severity, but it's a real message loss with no equivalent added elsewhere — worth restoring the printf so the "how do I fix this" hint survives the refactor.

Comment thread tests/full_validate.sh
printf 'Node.js is unavailable; the cross-language contract gate is incomplete.\n' >&2
else
printf 'Node.js is unavailable; the cross-language contract gate is incomplete.\n' >&2
return 2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior gap: standalone --gate contracts does less than the same gate run inside all

When $gate == "all" and Node is missing, run_contracts() sets node_contracts_missing=1 and falls through to still run generate_compatibility_dashboard.py --check and compileall (lines 69-70). But when --gate contracts is invoked standalone (e.g. from a future CI step or a developer's shell) and Node is missing, it return 2s immediately at line 67, skipping those same two checks entirely.

So the 'contracts' gate silently validates a different, smaller set of things depending on whether it's invoked alone or as part of the aggregate — a maintainer relying on ./tests/full_validate.sh --gate contracts for local iteration on a Node-less machine gets less coverage (no dashboard/compileall check) than they'd get from the full run, with no message indicating those two checks were skipped. Consider running the dashboard/compileall checks before deciding whether to return 2, so the standalone gate's coverage doesn't regress based on the node-missing branch taken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[v1.0] De-duplicate invariant gates across the test matrix

1 participant