fix(ci): collect the nooa-memory test suite 🤖🤖🤖 - #292
Open
sushant-mishra-dtu wants to merge 1 commit into
Open
Conversation
`testpaths` names packages/nooa-cli/tests, packages/nooa-acp/tests and
packages/nooa-bench/tests, but not packages/nooa-memory/tests. The CI unit-test
step runs pytest with no path arguments, so testpaths applies and the directory
is never reached — a shipped optional package with no regression protection.
The package is installed in CI (`uv sync --all-extras --no-extra sandbox` pulls
it in via the `memory` extra) and is a declared workspace member; only the
collection path was missing.
Measured with CI's exact invocation:
pytest --collect-only -q -m "not integration and not stress and not sandbox"
before: 7018 collected, 0 from nooa-memory
after : 7300 collected (+282)
Same shape as the ripgrep step added in NVIDIA-NeMo#65, which exists because otherwise
"those tests silently skip and the job passes vacuously".
Signed-off-by: sushant-mishra-dtu <sushant.arh@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe pytest configuration now includes ChangesPytest configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change expands the default pytest run to include nooa-memory coverage, with no remaining concrete merge-blocking risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 is wrong
testpathsinpyproject.tomllists three of the four packages underpackages/—nooa-cli,nooa-acp,nooa-bench— but notnooa-memory. The CI unit-test step(
.github/workflows/ci.yml:46) runswith no path arguments, so pytest applies
testpathsand never reachespackages/nooa-memory/tests. The suite is installed and importable in CI —uv sync --all-extras --no-extra sandboxpulls it in via thememoryextra(
pyproject.toml:88) and it is a declared workspace member (:72) — it is simplynever collected.
Net effect: a shipped optional package has no regression protection in CI. Nothing
in
nooa-memorycan fail the build.Evidence
With this one-line change, the same CI-shaped collection goes 7018 → 7300, exactly
the 282 that were dark.
Why this is worth doing now
There are four open PRs against
nooa-memoryright now — #277, #278, #279 and #284 —and each one adds or edits a file under
packages/nooa-memory/tests/. Thoseregression tests cannot currently fail, on those PRs or on any later change that breaks
them.
This is also a failure mode the project already treats as a bug.
ci.yml:40-44installsripgrep specifically because otherwise "those tests silently skip and the job passes
vacuously. See #65", and the release and gitleaks steps (
ci.yml:190,:234) bothhard-fail rather than pass vacuously. This is the same policy applied to a directory the
config never points at.
What I could and could not verify
I ran the suite on Windows, so my numbers are not a substitute for your runner:
All three failures look platform-specific rather than product defects:
tests/memory/test_memory_readme.py, which callsPath.read_text()with noencoding=and so fails on a cp1252 console — they pass underPYTHONUTF8=1, and wouldpass on a UTF-8 runner;
test_file_capture_rejects_escaping_pathsexpectsfile:/etc/passwdto take the"must be relative" branch, but
pathlib.Path("/etc/passwd").is_absolute()isFalseonWindows, so it takes the "escapes the working dir" branch instead. The path is still
rejected; only the message differs.
The full suite on this branch adds exactly those tests and moves nothing else. Both runs
behind the same harness, both under
PYTHONUTF8=1so they are comparable to each other:main@e137e1bThe delta — +1 failed, +268 passed, +13 skipped, errors unchanged — is exactly the
nooa-memorysuite's standalone result above, so the one-line change adds that suite andtouches nothing else. The absolute failure count is high only because this is Windows,
where a lot of the suite is known-broken (#84, #95, #110); the delta is the point, not the
level.
So I expect all 282 to be green on
ubuntu-latest, but I have not been able to provethat. If turning them on does surface something, say so and I will fix the fallout in
this PR rather than leave you with a red build.
Scope
One line. No source changes, no behaviour changes, no agent-facing contract touched.
Deliberately not included:
util/eval_pipeline/testsis the other test directoryoutside
testpaths. It is dev tooling underutil/rather than a shipped package, and Ihave not audited its state, so it seemed wrong to bundle it into a one-line fix. Happy to
look at it separately if you want it collected too.
Summary by CodeRabbit