test(scripts): add 19 tests for weekly_digest.py classify_notebook_changes (cycle 70)#1880
Conversation
…anges (cycle 70) Tests cover the pure function classify_notebook_changes: - Series detection (7): single/multiple series, name cap at 5, non-notebook files in series, backslash limitation, shallow paths - Other file types (6): python, csharp, docs, config, other, mixed - Notebook names (2): extraction and ordering - Edge cases (4): deeply nested paths, mixed separators, empty filename, set dedup semantics Notable finding: Windows backslash paths don't match startswith check before normalization — documented as known limitation via test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
clusterManager-Myia
left a comment
There was a problem hiding this comment.
[NanoClaw]
Solid test coverage for classify_notebook_changes -- 19 tests across 4 classes hit all branches of the pure function. No security concerns.
A few suggestions to strengthen the suite:
1. test_notebook_names_sorted_by_appearance only checks length
assert len(names) == 2 # doesn't verify ordering at allEither assert names[0] == "aaa" and names[1] == "zzz", or verify the sort property you're documenting. Right now this test passes even if the list is randomly ordered.
2. test_shallow_notebook_path doesn't verify the series name
The comment says series = "standalone.ipynb" but the assertion only checks len == 1. Add:
assert "standalone.ipynb" in result["series"]3. pytest imported but unused
The file imports pytest but never uses any pytest features (marks, fixtures, parametrize). Either add @pytest.mark.parametrize to the TestClassifyOtherFiles group (which would also reduce the 6 nearly-identical test methods), or drop the import.
4. sys.path.insert import hack
sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "notebook_tools"))This works but is fragile -- moving the test file or running from a different CWD breaks it. A conftest.py in scripts/tests/ that adds the path once, or a pyproject.toml [tool.pytest.ini_options] pythonpath, would be cleaner.
5. Backslash tests document a bug, not a feature
test_backslash_path_not_matched and test_notebook_path_with_mixed_separators both freeze current broken behavior where startswith("MyIA.AI.Notebooks/") fails on backslash paths even though the code does .replace(chr(92), "/") after the check. If this is intentional "frozen behavior," say so. If not, consider adding a TODO or linking an issue.
Overall: LGTM with minor polish. The coverage is good and the edge cases are thoughtful.
Summary
Add 19 unit tests for
classify_notebook_changesinweekly_digest.py— the pure function that classifies changed files by notebook series and file type.Test coverage
TestClassifySeriesDetectionTestClassifyOtherFilesTestClassifyNotebookNamesTestClassifyEdgeCasesNotable finding
Windows backslash paths don't match the
startswith('MyIA.AI.Notebooks/')check because it runs before thechr(92)normalization. Documented as a known limitation via test (test_backslash_path_not_matched).Quality check
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com