CI: gate the box and in-package CLI unit suites (1066 -> 2287 tests)#151
Merged
Conversation
The PR gate ran 1066 tests while two bodies of hardware-free tests already in the repo ran nowhere. test/unit/box/ (55 files, 1138 tests) was larger than every gated suite combined, and cli/tests/ held the gateway-auth, update-gate and box-storage coverage. Both now gate; the total is 2287. test/unit/box/ was excluded on the theory that GitHub-hosted runners resolved `lager` as a namespace package. That diagnosis was wrong and the problem was never runner-specific. About a dozen box test modules register a placeholder `lager` in sys.modules so they can load individual box files without executing the heavy package __init__, while twenty-two module-level `from lager ... import` statements need that __init__ to have run. Only alphabetical collection order reconciled the two -- a contract three modules document as load-bearing. Any -k, --ignore, explicit file list, or newly added earlier-sorting file broke collection with "cannot import name ... from 'lager' (unknown location)". test/unit/box/conftest.py now imports the real package once before any test module loads, supplies the two third-party modules that are neither guarded nor installed (flask_socketio and pygdbmi -- the latter with a real exception class, since debug/gdb.py catches GdbTimeoutError and catching a mock raises TypeError), and asserts `lager` resolved to the on-disk package so a regression fails at one line instead of cascading into twenty collection errors. Verified green three ways: alphabetical, reversed, and all 55 files run individually. Two tests that had never executed were wrong: - test_lock_state.py required an unrecognized holder_type to be coerced to `ephemeral`. lock_state.py preserves it verbatim on purpose -- other services write their own origin token, and coercing it would attach a TTL and let the reaper drop their reservation. - cli/tests/test_io_imports.py required lager.adc/dac/gpio to import. Those consolidated under lager.io.* and no longer exist. It now covers the supported surface, verifies lager.io re-exports are the same objects as the submodules, and asserts the removed aliases stay removed. cli/tests/test_box_lager_imports.py is excluded from collection: it contains no assert statements, so its 16 test_* functions passed unconditionally, including while importing modules that do not exist. It remains useful run directly. pytest is pinned so five required contexts cannot go red from an unrelated upstream release. `unit (box)` is a new status context. It reports but does not block until it is added to the branch ruleset, which must happen after this merges to main.
test/COVERAGE.md disagreed with disk on twelve file counts, contradicted itself twice about the size of test/unit/ and the MCP suite, listed two files that had been deleted, omitted thirty-seven that exist, and never mentioned CI at all -- so a reader could not learn that the largest test directory in the repo ran nowhere. Adds a "What runs in CI" section naming each workflow, its trigger, and whether it gates a PR, along with what is excluded and why. Corrects every count against disk, documents the previously missing files, and replaces the stale gap tables with a ranked list of CLI modules that no gated test exercises. The run instructions in CONTRIBUTING.md, README.md and test/README.md would give a contributor a red local run on green code: they omit PYTHONPATH, --import-mode=importlib and -c /dev/null, and run test/unit/ wholesale, which mixes the two suites that cannot share a pytest process. All three now match what CI does. Also fixes an unclosed code fence in test/README.md and its stale description of test/unit/ as containing only cli/.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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.
What this does
Takes the PR unit-test gate from 1066 tests to 2287. Two bodies of hardware-free tests already in the repo ran nowhere:
test/unit/box/cli/tests/unit (cli))test/unit/box/alone was larger than every previously gated suite combined. Seven of its files are actually CLI tests, socli/commands/box/config.py— 2507 lines, the biggest config parse/persist surface in the CLI — had 203 tests that no PR ever ran.Why the box suite was excluded, and why that reason was wrong
The workflow header blamed GitHub-hosted runners resolving
lageras a namespace package. There is no namespace package:box/lager/is a regular package andpip install -e cli/installs nothing namedlager.The real cause is self-inflicted and reproduces anywhere. About a dozen box test modules register a placeholder
lagerinsys.modulesso they can load individual box files without executing the heavy package__init__; twenty-two module-levelfrom lager ... importstatements need that__init__to have run. Only alphabetical collection order reconciled them — a contract three modules already document as load-bearing:test_binaries_store.pyimports the real package because "this file collects before test_box_http_server_capabilities.py"test_acroname_driver.py: "this file collects first alphabetically, so it used to break 7 other test modules"test_debug_net_user_scripts.pysets__path__ = []claiming it preserves submodule imports, whiletest_debug_net_self_heal.pywarns in a comment that exactly that "would shadow real submodules"Any
-k,--ignore, explicit file list, or newly added earlier-sorting file flipped it.test/unit/box/conftest.pyimports the real package once before any test module loads, stubs the only two third-party modules that are neither guarded nor installed (flask_socketio,pygdbmi— the latter with a real exception class, sincedebug/gdb.pycatchesGdbTimeoutErrorand catching a mock raisesTypeError), and assertslagerresolved to the on-disk package so a regression fails at one line instead of cascading into twenty collection errors.Defects this surfaced
Three, all invisible because the tests never executed:
test_lock_state.pyasserted the opposite of deliberate behavior. It required an unrecognizedholder_typeto be coerced toephemeral.lock_state.pypreserves it verbatim on purpose — other services write their own origin token, and coercion would attach a TTL and let the reaper drop their reservation. Test fixed and renamed; the old name was inaccurate.cli/tests/test_io_imports.pyrequired import aliases that no longer exist.lager.adc/dac/gpioconsolidated underlager.io.*. Every new path passed, every old one failed. Rewritten to cover the supported surface, verifylager.iore-exports are the same objects as the submodules, and assert the removed aliases stay removed.cli/tests/test_box_lager_imports.pyhas noassertstatements in 835 lines. Its 16test_*functions only log into a printed report, so under pytest they passed unconditionally — including while importing modules that do not exist. Excluded from collection viacli/tests/conftest.pyrather than gated as 16 checks that cannot fail. Still useful run directly.Verification
Box suite proven green three ways, since a single alphabetical run would not have shown the ordering dependency:
Also confirmed the gate actually fails: injected a bad assertion, watched
unit (box)go red, restored it, green again.pytestis now pinned. Previously all five required contexts floated on whatever released last night, and the--import-mode=importlibmodule-naming rules changed in 8.1.Merge mechanics
unit (box)is a new status context. It reports on this PR but does not block until it is added to ruleset 14535039 — and that must happen after this merges tomain, or every open PR shows a check that never reports. Rollback, if ever needed, is ruleset-first then workflow; the reverse order leaves PRs permanently pending.cli/tests/deliberately rides in the existingunit (cli)context rather than getting its own, so it needs no ruleset change.Second commit: docs
test/COVERAGE.mddisagreed with disk on twelve file counts, contradicted itself twice, listed two deleted files, omitted thirty-seven that exist, and never mentioned CI — so a reader could not learn that the largest test directory in the repo ran nowhere. It now has a "What runs in CI" section and counts checked against disk.The run instructions in
CONTRIBUTING.md,README.mdandtest/README.mdwould give a contributor a red local run on green code — they omitPYTHONPATH,--import-mode=importliband-c /dev/null, and runtest/unit/wholesale, which mixes the two suites that cannot share a pytest process. All three now match CI.