Expand unit test coverage to 94%, fixing four bugs found along the way - #8
Merged
Conversation
Coverage was 72% with four modules at 0%. This takes it to 94% (222 tests, ~70s) and covers every module that had none. New suites: * test_persist.py -- save/load round trip, the zip path and its cleanup, and the not-a-model / missing-path cases. * test_rootpath.py -- marker detection from files and nested directories, custom patterns, and the empty-directory quirk (see below). * test_log.py -- message formatting, levels, filtering and handlers. * test_const.py -- structural validation of the bundled dataset metadata (read() unpacks each field as a 4-tuple, so a malformed entry would only surface as an opaque error at read time) plus the label converter. * test_utils_plot.py / test_report_plot.py -- every plotting entry point, each asserting no matplotlib figure is left open. That guards the leak fixed in 381eb17 and caught two of the bugs below. Bugs found by the new tests: * trustee/utils/const.py: np.uint8(-1) raises OverflowError on NumPy 2 -- NumPy 1 wrapped it to 255. cic_ids_2017_label_converter() therefore blew up on any unrecognised label instead of returning its sentinel. 255 is now the explicit sentinel, preserving the NumPy 1 result. * trustee/report/plot.py: DataFrame.groupby(axis=1) was removed in pandas 2.0, so plot_distribution(aggregate=True) raised TypeError. Rewritten to group the bit columns by prefix explicitly, preserving column order so the bits keep their significance. This one survived the 1.2.0 upgrade because nothing exercised the path. * trustee/report/plot.py: class_names defaults to [] at seven indexing sites guarded only by `is not None`, which passes for an empty list. Calling any of those entry points without class_names raised IndexError, as did a class_names shorter than the tree's class count. Added a _class_label() helper doing the bounds check trust.py already did. * trustee/utils/plot.py: labels[i] was indexed unguarded in plot_stacked_bars while every other site in the file bounds-checks. Behaviour documented rather than changed: * rootpath.detect() returns None on the first empty directory it meets, so an empty subdirectory aborts the search before reaching the marker above it. * plot_distribution(aggregate=True) needs feature_names -- it converts X with .values, so DataFrame column names never reach the prefix regex. * skip_retrain=True reuses the blackbox as-is and so requires a fitted model. Determinism: Trustee.fit() samples via np.random.choice and calls train_test_split without a random_state, both drawing on NumPy's global RNG, which made any assertion on fidelity flaky. An autouse fixture now seeds it per test; the suite was run three times over to confirm. CI gains --cov-fail-under=90 so coverage cannot quietly regress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
os.path.expanduser resolves ~ from USERPROFILE on Windows (ntpath checks it first and never consults HOME), so setting only HOME left ~ pointing at the real profile directory, which has no root marker. Set both rather than skipping the test on one platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a `coverage-comment` job that posts a coverage summary to the PR once every test leg has passed. * `needs: test` gates it on the whole matrix, so a comment only ever appears for a run whose tests actually passed. * The report is rendered from coverage.json produced by the ubuntu/3.12 leg, which alone uploads the artifact -- otherwise six matrix legs race to write the same artifact name. * The comment is located by a hidden marker and updated in place, so pushing to a PR revises one comment instead of stacking a new one per run. The lookup matches on the marker *and* on github-actions[bot] as the author, so a comment that merely quotes the marker is never edited. * Skipped for forks, whose GITHUB_TOKEN is read-only and cannot comment. .github/scripts/coverage_summary.py renders the Markdown: overall percentage against the floor, a bar, and a table of partially covered files sorted worst-first with their uncovered line ranges condensed. Fully covered files collapse into a <details> line rather than padding the table. Posting uses the gh CLI already on the runner rather than another third-party action, and the jq lookup was checked against the live API and against fixtures covering the match, no-match and impostor-author cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report✅ 93.51% meets the 90% floor.
5 file(s) at 100%
Measured on Python 3.12, ubuntu-latest. |
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.
Summary
Coverage was 72% with four modules at 0%. This takes it to 94% — 222 tests, ~70s — and gives every module some.
utils/persist.pyutils/rootpath.pyutils/log.pyutils/const.pyreport/plot.pyutils/plot.pyreport/trust.pyutils/dataset.pymain.pyBugs the new tests found
Writing tests for the untouched paths turned up four real defects — two of them upgrade fallout that survived 1.2.0 because nothing exercised the code.
np.uint8(-1)raisesOverflowErroron NumPy 2. It wrapped silently to 255 on NumPy 1, socic_ids_2017_label_converter()blew up on any unrecognised label instead of returning its sentinel. 255 is now explicit, preserving the old result.DataFrame.groupby(axis=1)was removed in pandas 2.0, soplot_distribution(aggregate=True)raisedTypeError. Rewritten to group bit columns by prefix explicitly, preserving column order so the bits keep their significance. I verified the arithmetic separately rather than just asserting it runs —flags_0/flags_1fold to[1, 2, 3, 0].class_namesdefaults to[]at seven indexing sites guarded only byis not None— which passes for an empty list. Calling any of those entry points withoutclass_namesraisedIndexError, as did aclass_namesshorter than the tree's class count. Added a_class_label()helper doing the bounds checktrust.pyalready did.labels[i]was indexed unguarded inplot_stacked_bars, while every other site in that file bounds-checks.Behaviour documented, not changed
Three surprises I left alone but pinned with tests, since changing them is a judgement call:
rootpath.detect()returnsNoneat the first empty directory it meets — an empty subdirectory aborts the search before reaching the marker above it.plot_distribution(aggregate=True)requiresfeature_names; it convertsXwith.values, so DataFrame column names never reach the prefix regex.skip_retrain=Truereuses the blackbox as-is and therefore requires an already-fitted model.Flakiness
Trustee.fit()samples vianp.random.choiceand callstrain_test_splitwithout arandom_state— both draw on NumPy's global RNG. My first fidelity assertion passed, then failed on a later run. An autouse fixture now seeds per test; I ran the full suite three times over to confirm it's stable.Worth flagging separately: the library itself is nondeterministic by default.
Trustee(...).fit()gives different explanations across runs unless the caller seeds NumPy globally. That may be intended for the stability analysis, but there's norandom_stateparameter to pin it. Happy to add one if you want it.Also
--cov-fail-under=90so coverage can't quietly regress.ndarray.shapedirectly, which NumPy 2.5 deprecated. It fires on everyload_model()and there's no version to upgrade to. Scoped to the exact message soDeprecationWarningstays an error otherwise — worth watching, since it becomes a hard break when NumPy removes it.