Skip to content

ci(vendor): test the regenerated tree before committing it (#1853) - #1879

Open
JarryShaw wants to merge 1 commit into
mainfrom
ci/1853-vendor-test-before-commit
Open

JarryShaw wants to merge 1 commit into
mainfrom
ci/1853-vendor-test-before-commit

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

  • cron-vendor.yml: a step after Update Vendor installs the test extra and runs
    tests/const and tests/vendor through util/vendor_test_gate.py, before
    anything decides what changed; continue-on-error, with a timeout above the
    script's budget, and a next step that discards every regenerated module if
    it did not finish.
  • util/vendor_test_gate.py: per python -m pcapkit.vendor exits 0 when a crawler raises, so a no-op regeneration looks like success #872, keeps the modules the tests pass with and
    puts the rest back as HEAD has them, found by re-running the tests on halves
    of the changed modules over HEAD's tree; discards all of them when HEAD fails
    too, the kept set fails together, or the budget runs out. Runs are capped on
    file size and resident memory.
  • test_vendor_test_gate_1853.py: the step's place, install, continue-on-error
    and timeout, read with PyYAML; the discard step; the script on scratch repos.
  • test_isort_clean.py: its docstring no longer says the job runs no tests.

Closes #1853

Unverified until the next scheduled run on macos-latest: whether ps -A -o pgid= -o rss= behaves there as it does here, and whether a step timeout counts as a failed step under continue-on-error. If tests/const or tests/vendor fail at HEAD on that runner, every regeneration is discarded with a warning.

@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 57f2bd9f8. A second Sonnet review (the first stalled and was stopped) confirmed that the gate and its wiring work. What it needs is prose that this PR makes false:

  1. pyproject.toml:309 says cron-vendor.yml "never invokes pytest". After this PR it installs .[test] and runs tests/const and tests/vendor through the gate. Reword it, and re-check the rest of that comment, which argues from the same premise.
  2. tests/_dependency_gates.py:120-124 lists "a vendor crawl" among the workflows where "none of them invokes the suite". The pytest_jobs docstring at :1433-1436 makes the same claim. While fixing these, drop the counts in that docstring ("five other workflows", "three of them").

The review missed item 2; I found it myself.

What it confirmed:

  • Bisection. On scratch repos with a stub test command, the gate never kept a failing module or restored a passing one, across every bad subset of up to 7 modules and modules that fail only together. A failing HEAD discards everything.
  • Budget and caps. A budget that runs out leaves the tree identical to HEAD. The memory cap kills a runaway process group, and the file-size cap reaches the child.
  • Wiring. steps.vendor-tests.outcome is the right test under continue-on-error, per GitHub's contexts docs. The implicit success() means no path commits an unfinished tree.
  • Tests. The test file catches 15 of 16 mutations of the gate and the workflow. The survivor is equivalent behaviour.

Unverified: macOS ps -o pgid= -o rss=, and whether a step timeout counts as a failed step. The wiring is safe either way.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
- cron-vendor.yml: a step after Update Vendor installs the test extra and runs
  tests/const and tests/vendor through util/vendor_test_gate.py, before
  anything decides what changed; continue-on-error, with a timeout above the
  script's budget, and a next step that discards every regenerated module if
  it did not finish.
- util/vendor_test_gate.py: per #872, keeps the modules the tests pass with and
  puts the rest back as HEAD has them, found by re-running the tests on halves
  of the changed modules over HEAD's tree; discards all of them when HEAD fails
  too, the kept set fails together, or the budget runs out. Runs are capped on
  file size and resident memory.
- test_vendor_test_gate_1853.py: the step's place, install, continue-on-error
  and timeout, read with PyYAML; the discard step; the script on scratch repos.
- test_isort_clean.py, tests/_dependency_gates.py and the test extra's comment
  in pyproject.toml no longer say the vendor job runs no tests.

Closes #1853
@JarryShaw
JarryShaw force-pushed the ci/1853-vendor-test-before-commit branch from 57f2bd9 to 10f37ed Compare October 17, 2026 02:07
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 10f37ed81. The Sonnet reviewer re-checked the changes since 57f2bd9f8.

  • pyproject.toml no longer says the vendor job never invokes pytest. The new wording matches the workflow:
    • it installs .[all,vendor] first and then .[test];
    • the gate runs the tests only when a crawl changed a module under pcapkit/const/;
    • by then pcapkit-vendor has rewritten the IPX file from its DATA table.
  • tests/_dependency_gates.py now describes the vendor job correctly, and the counts in the pytest_jobs docstring are gone. The code is unchanged: the file's AST is identical with docstrings and comments stripped.
  • test_vendor_test_gate_1853.py, test_tier_guard.py, test_sweeps_on_latest_only_1732.py and test_conventions_doc_claims.py all pass.

Nit, not a blocker: the new pyproject.toml sentence ends "needs nothing from here to", with a dangling "to".

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026

This branch has not been deployed

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

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: good-to-go Cross-review at the current head says ready; CI state is separate

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

ci(vendor): the vendor-update leg commits regenerated const data without testing it

1 participant