chore(ci): fail when a test suite is executed by nothing (DEBT-031)#110
Merged
Conversation
`make test` and `ci-unit.yml` enumerate the packages in two independent hand-maintained lists, and the DEBT-029 guard checks only that a package is isolated, never that anyone runs it. A suite absent from both lists was silently never executed and nothing went red. That is the hole BUG-024 fell through, still open one level up. Add `vektra-shared/tests/test_suite_execution_coverage.py`: it parses the `test:` recipe and every workflow's pytest invocations and asserts each test file would actually be *collected* — honouring the `-m` filter, not just the paths. That distinction is the point: a directory-level check passes, because `vektra-admin/tests/` is named in both lists; what nobody ran was the `integration`-marked file inside it, excluded from every unit run by `-m "not integration"`. Unit and integration paths are checked independently. YAML is parsed, never grepped, so the commented-out pytest line in integration.yml grants no coverage. It runs in a `test-structure` job with no path filter, deliberately: every other unit job is gated on paths-filter, so a Makefile-only edit, a workflow-only edit, or a new package skips them all — exactly the three changes the guard exists to catch. The DEBT-029 guard runs there too. Pointed at develop, it found five suites executed by nothing: `tests/test_startup.py` (ARCH-057 startup validation; its docstring assumed integration.yml ran it, which names tests/integration/ and never tests/), and the integration suites of vektra-admin, vektra-index (x2) and vektra-ingest, left behind when DEBT-030 wired up vektra-app by hand. All now run. The four had not rotted (52 integration tests pass), which was luck, not design. `app-integration` becomes `package-integration` over a `vektra-*/tests` glob, so the package list stops being hand-maintained. The five suites under tests/ also gain the `integration` marker they require; `tests/nfr/test_performance.py` stays deliberately unrun (needs a live LLM) as the one justified entry in the guard's EXPECTED_UNRUN. Required check names are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fvadicamo
added
component:shared
github-actions
Bot
added
the
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCI now marks live-stack suites as integration tests, runs startup and package-wide integration coverage, rejects empty LLM provider configuration, and adds a static guard that verifies test suites are collected by Makefile and GitHub Actions commands. ChangesCI test coverage enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IntegrationWorkflow
participant PackageIntegration
participant Pytest
participant IntegrationGate
IntegrationWorkflow->>Pytest: run startup and integration tests
IntegrationWorkflow->>PackageIntegration: start package-wide integration job
PackageIntegration->>Pytest: collect vektra-*/tests with -m integration
IntegrationGate->>IntegrationWorkflow: check integration result
IntegrationGate->>PackageIntegration: check package-integration result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses DEBT-031 by introducing a structural guard (test_suite_execution_coverage.py) that ensures all test suites are executed by either local or CI runners, preventing silent test rot. It also marks several previously unrun or unmarked integration test suites with pytest.mark.integration so they are correctly classified and executed. The reviewer identified an issue in the AST parsing logic of the new guard, which fails to detect the integration mark when applied at the class level rather than the module or function level, and provided a code suggestion to resolve this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The classifier read only module-level `pytestmark` and function decorators. A test class carrying `@pytest.mark.integration` (or a class-body `pytestmark`) would therefore be classified as unit: the guard would never demand an integration run for it, and every unit run would deselect it by `-m "not integration"`. Silently unexecuted — the exact failure this file exists to prevent, reproduced inside the guard itself. Class-based tests are already used here (vektra-admin), so the hole was live. Marks are now inherited module -> class -> function, as pytest applies them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-unit.yml:
- Around line 209-224: Update the checkout step in the test-structure job to set
persist-credentials to false, matching the credential-isolation behavior of
test-app before running the PR-authored structural tests.
In `@vektra-shared/tests/test_suite_execution_coverage.py`:
- Line 33: Declare PyYAML as a dependency of the vektra-shared package in its
pyproject.toml so the yaml import in the test suite remains available when
syncing only that package’s dev dependencies. Use the package’s existing
dependency configuration and preserve the current test execution flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53ac895d-21c2-481d-9ea1-71536b2497bb
⛔ Files ignored due to path filters (1)
.s2s/BACKLOG.mdis excluded by!.s2s/**
📒 Files selected for processing (9)
.github/workflows/ci-unit.yml.github/workflows/integration.ymlCHANGELOG.mdtests/integration/test_chunk_lifecycle.pytests/integration/test_e2e_flow.pytests/nfr/test_nfr_hard.pytests/nfr/test_performance.pytests/test_startup.pyvektra-shared/tests/test_suite_execution_coverage.py
`llm_provider` is required but typed as a bare `str`, and `""` is a valid `str`, so `VEKTRA_LLM_PROVIDER=` was accepted: pydantic never raised, ARCH-057 step 1 never fired, and the stack booted and served with an empty LLM provider, deferring the misconfiguration to the first query. That is the exact failure mode the startup sequence exists to prevent (REQ-011, NFR-009). `min_length=1` makes an empty value fail at step 1 with the same structured, remediable error an absent one already produced. Found by running tests/test_startup.py for the first time (DEBT-031). The test has asserted this behaviour since it was written, believing `docker run -e VAR=` *unsets* the variable when it sets it to the empty string — so it was asserting against a value the product happily accepted, and hung waiting for a container that had booted fine. Nothing ever ran it, so nobody found out. Fixed at the source rather than by weakening the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uctured error With the empty-provider fix in place, tests/test_startup.py exposes the next layer: step 1 emits the structured [STARTUP ERROR] block, and then `raise SystemExit(1)` travels out of the ASGI lifespan into uvicorn, which logs the exception. The operator who typo'd an env var gets the good message *and* a Python traceback. NFR-009 asks for the former instead of the latter, not both, and it applies to all 11 steps, not just config validation. The fix is to validate before uvicorn.run() rather than inside the lifespan -- the boot path BUG-024 broke, and vektra-app's own tests drive that lifespan in-process and expect SystemExit. Too much to bolt onto a PR about test execution, so it is filed as BUG-025 with the analysis and the trap written down. The assertion is split into its own test with xfail(strict=True): the defect stays visible and CI stays honest, and when BUG-025 lands the test XPASSes and the suite goes red until the marker is removed. A known defect that cannot rot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pyyaml Two verified review findings from the PR bots: - test-structure ran PR-authored test code without persist-credentials: false, unlike test-app and app-integration which strip the token from .git/config for exactly that reason. Same threat model; same fix. - test_suite_execution_coverage.py imports yaml, but vektra-shared did not declare pyyaml — it relied on the root dev group leaking into a `uv sync --package vektra-shared`. It works (both test-shared and test-structure were green), but an implicit transitive dependency for a guard is the precise fragility this PR is about, so declare it where it is used. A third bot comment (class-level integration marks) was already fixed in 1b2ceef before the comment landed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Nothing guaranteed a test suite was ever executed.
make testandci-unit.ymlenumerate the packages in two independent hand-maintained lists, and the DEBT-029 guard checks only that a package is isolated, never that anyone runs it. A suite absent from both lists was silently unexecuted and no test went red — the exact shape of the hole BUG-024 fell through, still open one level up.This adds
vektra-shared/tests/test_suite_execution_coverage.py, which reads the runners instead of trusting them.What the guard does
It parses the
test:recipe in the Makefile and everypytestinvocation in every workflow, then asserts that each test file would actually be collected by one — honouring the-mfilter, not just the paths.That distinction is the whole thing. A directory-level check (which is what the backlog originally proposed) would have passed:
vektra-admin/tests/is named in both lists. What nobody ran was theintegration-marked file inside it, excluded from every unit run by-m "not integration". So the unit path and the integration path are checked independently.Two design points worth reviewing:
integration.ymlcarries a commented-outpytestline fortest_performance.py. A guard that grepped the text would count it as coverage and certify a suite nobody runs.test-structurejob carries no path filter, deliberately. Every other unit job is gated ondorny/paths-filter, so a PR that only edits the Makefile, only edits a workflow, or adds a brand-newvektra-foo/matches no filter and skips them all — and those are precisely the three changes this guard exists to catch. A guard a path filter can skip is not a guard, so the DEBT-029 guard (until now gated onvektra-shared/**) runs there too.Found the moment it was pointed at develop: five suites executed by nothing
tests/test_startup.pytests/integration/, and nothing ever namedtests/. It was watching the precise surface BUG-024 broke, and it was watching nothing.vektra-admin/tests/test_integration.pyintegration-marked → excluded from every unit run; the only job running-m integrationnamedvektra-app/tests/alonevektra-index/tests/test_integration.pyvektra-index/tests/test_benchmark.pyvektra-ingest/tests/test_integration.pyDEBT-030 wired up
vektra-appby hand and left the identical hole open in three neighbouring packages.Unlike vektra-app's suites in DEBT-030, these four had not rotted — 52 integration tests pass — but that was luck, not a property of the arrangement.
Fixes that make it green
app-integration→package-integration, targetingvektra-*/tests -m integration. A glob on purpose: the hand-written package list is the thing that has now failed twice, so a package added tomorrow is picked up with no edit.tests/test_startup.pyjoins the job that already brings the stack up.tests/gain theintegrationmarker they require. Unmarked, they were indistinguishable from unit testsmake testhad merely forgotten — which is exactly why nobody noticed.tests/nfr/test_performance.pystays deliberately unrun (it needs a live LLM that GitHub Actions does not have) as the single justified entry in the guard'sEXPECTED_UNRUN, where a reviewer can see it.Proof it works — broken five ways, red each time
A guard you have not watched fail is not a guard.
vektra-indexfrom the CI workflowtest_every_unit_suite_runs_in_civektra-indexfrom the Makefiletest_every_unit_suite_runs_in_make_testvektra-app(the DEBT-030 regression)test_every_integration_suite_runs_in_civektra-foo/tests/nobody wired uppytestline as coverageEach went green again on restore.
Branch protection
The required check names are unchanged:
CI gateandIntegration tests + NFR gates. The newtest-structurejob is added toci-gate'sneeds; the renamedpackage-integrationis added tointegration-gate's. Neither aggregator'sname:was touched.Verification
make lintcleanmake test: 771 passed, 3 skipped, 52 deselecteduv run pytest vektra-*/tests -m integration: 52 tests collected across all four packages (duplicatetest_integration.pybasenames handled by--import-mode=importlib)Closes DEBT-031.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
VEKTRA_LLM_PROVIDERis rejected.Tests / CI
Documentation