test(scripts): add 56 tests for validate_sc_notebooks.py (cycle 71)#1882
Conversation
Test pure functions: source_text (7), is_nav_cell (7), has_exposed_solution (8), make_source_list (5), get_executable_cells (12), validate_notebook (14), dataclasses (2), Colors (1). Key invariants: nav link detection, solution exposure heuristics, executable cell filtering (TODO/NotImplementedError/pip skip, anvil profile gating), structural validation (title/nav/objectives/duration, STRING format, consecutive code, anvil web3 requirement). Suite: 1008 passed (0 regression). 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 file overall — good coverage of the pure functions in validate_sc_notebooks.py with well-organized class-based grouping and proper use of tmp_path fixtures. A few issues worth addressing:
1. sys.path.insert hack (line 14)
sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "smartcontracts"))This is fragile and won't work if tests are invoked from a different working directory or by a test runner that copies files. Prefer:
- A
conftest.pyatscripts/tests/conftest.pythat adds the import path once - Or add
scripts/smartcontractstotestpaths/pythonpathinpyproject.tomlunder[tool.pytest]
2. TestColors.test_disable — fragile global state (lines 498-506)
This test mutates class-level attributes (Colors.GREEN, Colors.RED, etc.) and tries to restore them afterward, but:
- Only
Colors.GREENis saved/restored from the original value; the rest (RED,YELLOW,BLUE,CYAN,BOLD,END) are hard-coded back to their expected ANSI codes - If any other test runs in parallel and reads
Colorsduring the restore window, it sees inconsistent state - Tests for ANSI color helpers are generally low-value — they test that string constants exist, not behavior
Suggestion: Either remove this test class entirely (it tests that class attributes can be set to ""), or refactor Colors to be instance-based so tests can isolate state without global mutation.
3. Dataclass tests are tautological (lines 485-496)
test_notebook_validation_defaults and test_cell_info_creation just verify that Python's dataclass __init__ assigns fields correctly. These tests will never break unless Python itself is broken. Consider either:
- Removing them (no real value)
- Or testing meaningful behavior (e.g., serialization, equality, or how defaults interact with validation logic)
4. Minor: test data naming mismatch (line 82-84)
def test_list_with_trailing_newline(self):
cell = {"source": ["line1\n", "line2\n", ""]}
assert source_text(cell) == "line1\nline2\n"Comments/docstrings reference "hello/world" in surrounding tests but this one uses "line1/line2" — cosmetic but slightly confusing when reading the class as a whole.
5. Count discrepancy
Title says 56 tests. I count 59 test methods across 8 classes (7+7+8+5+12+17+2+1). Minor — worth fixing the title to match.
6. Missing edge case coverage
make_source_listwith\r\n(Windows line endings) — no test for this, and the split logic (splitting on\n) may produce\rartifactsget_executable_cellswithfoundryprofile —standaloneandanvilare tested, butfoundryis only tested implicitly intest_profile_stored. Afoundry-specific filtering test would close the gap
Verdict
Good structural quality and meaningful assertions for the core validation logic. The main actionable items are the sys.path hack (#1, should fix before merge) and the fragile Colors test (#2, consider dropping). The rest are suggestions.
Summary
Add 56 tests for
scripts/smartcontracts/validate_sc_notebooks.py— SC notebook structural validator.Functions tested
source_textis_nav_cellhas_exposed_solutionmake_source_listget_executable_cellsvalidate_notebookTest count
Scope
scripts/tests/test_validate_sc_notebooks.pyCo-Authored-By: Claude Opus 4.8 noreply@anthropic.com