Skip to content

ci(tests): run tests/vendor in the unittest-ordering job (#981, #985) - #988

Merged
JarryShaw merged 2 commits into
mainfrom
ci/981-reenable-vendor-ordering-leg
Oct 2, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
ci/981-reenable-vendor-ordering-leg

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • 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
  • chore — anything else

Description of your pull request and other information

Part of #981. tests/vendor was left out of the unittest-ordering matrix because it held an instance of the defect that job exists to catch (#985, a stale VendorRuntimeWarning generation failing assertWarnsRegex). That defect is fixed on main, so the exclusion no longer has a reason.

  • Adds vendor to the leg matrix (now 10 legs, timeout-minutes still 45).
  • Removes the tests/vendor bullet from the exclusion comment, changes "Three" to "Two", and records why the directory is now covered. tests/corekit and tests/integration bullets are untouched.
  • util/run_unittest_leg.py does not name the exclusion; unchanged.

Evidence: python util/run_unittest_leg.py vendor from this branch (pcapkit.__file__ confirmed inside the worktree) printed tests/vendor + 5 root module(s): 297 test(s), 0 failure(s), 0 error(s), 105.3s elapsed. yaml.safe_load: 5 top-level keys, 8 jobs; only unittest-ordering differs from main.

- tests/vendor was excluded from the unittest-ordering matrix because it
  held a stale VendorRuntimeWarning generation (#985); that defect is fixed.
- Add `vendor` to the `leg` matrix and drop its bullet from the exclusion
  comment; the count of excluded directories goes from three to two.
- tests/corekit and tests/integration stay excluded, bullets unchanged.
- util/run_unittest_leg.py does not name the exclusion; no change there.
@JarryShaw JarryShaw added ci Pull requests that change CI or workflow configuration (ci: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 9529cd129 — opus cross-review. The CI change is correct and the leg is
deterministically green; the one defect is a figure in the wrong units.
The reviewer was explicit that it did
not want to label a green, correct workflow change as NEEDS CHANGES on its code, and said so rather than
picking for me. I am taking the stricter reading, because a wrong number in the comment that exists to size
timeout-minutes is exactly the defect class every round on #984 caught.

vendor 83s is the directory measured alone. The leg is ~104s. I verified the units myself rather than
accepting the inference: the comment's wall-time table says cli 18s, and leg_modules('cli') returns one
module against root_modules()'s 5. A single module cannot take 18s when the whole 14-module vendor
directory takes 83s — so the table is leg-scoped. The reviewer pinned it harder: cli directory-alone is
0.0s / 6 tests against a leg of 18.3s / 185 tests. So a maintainer reading … utilities 42s and then
vendor 83s reads 83s as vendor's leg cost, and is 25% low. Two aggravating details: the parenthetical labels
the count as "directory" but leaves the time unqualified, and the table still enumerates 9 legs while
saying "one leg per matrix cell" with 10 in the matrix. Being fixed.

The strongest result is not the YAML — it is that the defect class is structurally absent from tests/vendor,
not merely unobserved.
That was my sharpest question and the answer is better than "we fixed the one we
found". By AST sweep across all 14 modules, including statements nested in module-level try/if/with and
in class bodies: zero module-level pcapkit-reaching imports. Every module-level mention of pcapkit is a
docstring, a find_spec against a third-party name, a filesystem path, or a tuple of strings. So there is no
import-time binding for a sibling's purge to desync. The only two generation-sensitive assertions resolve their
class in setUp — the #985 fix, and test_vendor_dest_path_unit.py which purges first and then imports. All
ten purge_modules call sites spell it purge_modules(['pcapkit']), confirmed by AST rather than by eye. No
guard is needed and re-enabling now is right.

required-checks does not interact at all, which the author had left open. unittest-ordering is absent
from its needs: before and after, and its step hard-codes the same four jobs, so a matrix cell cannot reach
it. The new cell does add a reported context, Plain unittest ordering (vendor), which is non-required — red
without blocking. No ruleset change needed. Timeout headroom is large: 104s against a 45-minute cap, with
protocols still the only binding leg.

Nothing would make the leg flaky: order-deterministic via sorted() in both leg_modules and
root_modules, no network on the tested paths, no setUpClass-cached pcapkit state. Two residual risks it
named are pre-existing and not this change's: a CI image missing html5lib would silently skip rather than
fail, and one test exercises a PermissionError path that would behave differently as root.

A correction to my own briefs: I have been stating the venv as Python 3.14.7 in every agent brief. It is
3.14.8. Harmless so far, but it was wrong every time.

UNVERIFIED by it: no run on ubuntu-latest, so all timings are this machine; the YAML structural checks were
taken from my brief rather than re-derived; the ruleset was not read from the settings API; and the table's other
seven leg figures remain unmeasured — though the cli measurement alone settles the units question, which is
what the finding turned on.

…comment (#981)

- The `vendor 83s, 118 directory tests` parenthetical measured tests/vendor
  alone, but the same comment block's wall-time table uses the identical
  `<name> Ns` shape for whole-leg times, so a reader sizing `timeout-minutes`
  read 83s as vendor's leg cost and was 25% low -- in the one comment that
  exists to size that cap. The leg is 104s / 297 tests; 118 of those are
  tests/vendor's own, the other 179 the five root-level modules every leg
  carries.
- The wall-time table enumerated nine legs while saying "one leg per matrix
  cell", and the matrix carries ten: `vendor 104s` was never added. It is now,
  in the list's existing alphabetical order.
- "re-resolving the class fresh per generation" is now "per test", which is
  what yields the current generation.

Comments only: `yaml.safe_load` of the file parses to a document identical to
the previous revision's (sha256 5989269..., eight jobs each structurally
unchanged, `leg` still ten entries, `timeout-minutes` still 45).
@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 2, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at b31f16b38. The units defect is fixed and I verified the change is comment-only by my own
yaml.safe_load comparison — the parsed documents are deep-equal to 9529cd129, 8 jobs, leg still 10
entries with vendor, timeout-minutes still 45. 9 changed lines, all inside # comments.

vendor 104s now sits in the wall-times list in alphabetical order, and the parenthetical is re-scoped to
"297 tests in the leg, 118 of them tests/vendor's own". It also took the optional improvement and wrote
"fresh per test" rather than "per generation", keeping "generation" in the sentence above where it describes
the stale warning — which is the more accurate reading.

It confirmed the leg-scoped inference three ways rather than taking it on trust, and the structural one is
the cleanest: root_modules() returns 5 modules contributing 179 tests, and every leg count is exactly
directory + 179 — cli 185, dumpkit 199, interface 201, vendor 297, const 478. It also noticed the script's own
docstring pairs the same figures ("dumpkit (199 tests, ~39s)"), so the table and the docstring are one
leg-scoped set.

One honest flag from it I am accepting rather than acting on: its own vendor run was 105.758s, so with
my 104.2s and 105.3s the spread is 104.2-105.8 and 104s is the low end rather than the centre (~105s). It
measured on a host at 81% disk with ~50 sibling worktrees, and the block's own prose notes contended runs
inflating 1.22-1.31×, so the quieter figure is the fairer one for a comment that sizes a 45-minute cap. Not
worth another round.

And it found a stale claim in a file it does not own, which I verified and which predates this pull
request.
docs/source/contributing/workflows.rst:206-214 says gate-only: true selects two of seven
jobs and skips the other five, enumerating test, integration, engine-tests, pypcap-parity and
required-checks. Measured against the workflow: there are eight jobs, and unittest-ordering carries
if: ${{ inputs.gate-only != true }} like the other four — so it is two of eight, skipping six, and
unittest-ordering is missing from the list entirely. That went stale when #984 added the eighth job, not
here. Fixing it separately as a docs-only change.

UNVERIFIED by it: the eight other table figures — it verified their scope via the docstring pairing and the
directory + 179 arithmetic but re-timed none; the RSS figures and the 29 GB OOM claim; and the contended-run
numbers behind timeout-minutes: 45.

@JarryShaw
JarryShaw merged commit efc3b08 into main Oct 2, 2026
73 checks passed
@JarryShaw
JarryShaw deleted the ci/981-reenable-vendor-ordering-leg branch October 2, 2026 15:57
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 2, 2026
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant