Add base test coverage#37
Conversation
df68bf1 to
17b9061
Compare
SamWilsn
left a comment
There was a problem hiding this comment.
Partial review. 18 more files to go!
| @@ -0,0 +1,173 @@ | |||
| # Copyright (C) 2025 Ethereum Foundation | |||
| @pytest.fixture | ||
| def temp_dir() -> Iterator[Path]: | ||
| with tempfile.TemporaryDirectory() as td: | ||
| yield Path(td) |
There was a problem hiding this comment.
Would https://docs.pytest.org/en/stable/how-to/tmp_path.html be useful here?
| _path: PurePath | ||
|
|
||
| def __init__(self, path: Optional[PurePath] = None) -> None: | ||
| self._path = path if path is not None else PurePath("mock.py") |
There was a problem hiding this comment.
| self._path = path if path is not None else PurePath("mock.py") | |
| self._path = PurePath("mock.py") if path is None else path |
The negation adds cognitive load, IMO
| settings = Settings(temp_dir, {"tool": {"docc": {}}}) | ||
| plugin_settings = settings.for_plugin("test") |
| class TestDiscover: | ||
| def test_concrete_discover(self, temp_dir: Path) -> None: | ||
| settings = Settings(temp_dir, {"tool": {"docc": {}}}) | ||
| plugin_settings = settings.for_plugin("test") | ||
|
|
||
| discover = ConcreteDiscover(plugin_settings) | ||
| sources = list(discover.discover(frozenset())) | ||
|
|
||
| assert len(sources) == 1 | ||
| assert isinstance(sources[0], MockSource) |
There was a problem hiding this comment.
Is this intentionally testing ConcreteDiscover, like the one defined in this file?
There was a problem hiding this comment.
Yes, indeed it was. Overkill. Removed this.
| class TestBlankNode: | ||
| def test_children_returns_empty_tuple(self) -> None: | ||
| node = BlankNode() | ||
| assert tuple(node.children) == () | ||
|
|
||
| def test_replace_child_raises_type_error(self) -> None: | ||
| node = BlankNode() | ||
| with pytest.raises(TypeError): | ||
| node.replace_child(BlankNode(), BlankNode()) | ||
|
|
||
| def test_repr(self) -> None: | ||
| node = BlankNode() | ||
| assert repr(node) == "<blank>" | ||
|
|
||
| def test_bool_is_false(self) -> None: | ||
| node = BlankNode() | ||
| assert bool(node) is False |
There was a problem hiding this comment.
I feel like there are some base operations that all nodes need to implement. Would parameterization help here?
| class RecordingVisitor(Visitor): | ||
| events: List[str] | ||
|
|
||
| def __init__(self) -> None: | ||
| self.events = [] | ||
|
|
||
| @override | ||
| def enter(self, node: Node) -> Visit: | ||
| self.events.append(f"enter:{repr(node)}") | ||
| return Visit.TraverseChildren | ||
|
|
||
| @override | ||
| def exit(self, node: Node) -> None: | ||
| self.events.append(f"exit:{repr(node)}") | ||
|
|
||
|
|
||
| class SkippingVisitor(Visitor): | ||
| events: List[str] | ||
|
|
||
| def __init__(self) -> None: | ||
| self.events = [] | ||
|
|
||
| @override | ||
| def enter(self, node: Node) -> Visit: | ||
| self.events.append(f"enter:{repr(node)}") | ||
| return Visit.SkipChildren | ||
|
|
||
| @override | ||
| def exit(self, node: Node) -> None: | ||
| self.events.append(f"exit:{repr(node)}") |
There was a problem hiding this comment.
| class RecordingVisitor(Visitor): | |
| events: List[str] | |
| def __init__(self) -> None: | |
| self.events = [] | |
| @override | |
| def enter(self, node: Node) -> Visit: | |
| self.events.append(f"enter:{repr(node)}") | |
| return Visit.TraverseChildren | |
| @override | |
| def exit(self, node: Node) -> None: | |
| self.events.append(f"exit:{repr(node)}") | |
| class SkippingVisitor(Visitor): | |
| events: List[str] | |
| def __init__(self) -> None: | |
| self.events = [] | |
| @override | |
| def enter(self, node: Node) -> Visit: | |
| self.events.append(f"enter:{repr(node)}") | |
| return Visit.SkipChildren | |
| @override | |
| def exit(self, node: Node) -> None: | |
| self.events.append(f"exit:{repr(node)}") | |
| class RecordingVisitor(Visitor): | |
| returns: ClassVar[Visit] = Visit.TraverseChildren | |
| events: List[Tuple[Literal["enter", "exit"], int]] | |
| def __init__(self) -> None: | |
| self.events = [] | |
| @override | |
| def enter(self, node: Node) -> Visit: | |
| self.events.append(("enter", id(node))) | |
| return self.returns | |
| @override | |
| def exit(self, node: Node) -> None: | |
| self.events.append(("exit", id(node))) | |
| class SkippingVisitor(RecordingVisitor): | |
| returns: ClassVar[Visit] = Visit.SkipChildren |
Two major suggestions: use id instead of repr because you can check object identity, and inherit instead of re-implementing.
Untested!
| visitor = RecordingVisitor() | ||
| node.visit(visitor) | ||
|
|
||
| assert visitor.events == ["enter:<blank>", "exit:<blank>"] |
There was a problem hiding this comment.
Then this would look something like:
| assert visitor.events == ["enter:<blank>", "exit:<blank>"] | |
| assert visitor.events == [ | |
| ("enter", id(node)), | |
| ("exit", id(node)), | |
| ] |
| [tool.pytest.ini_options] | ||
| # term-missing shows uncovered line numbers in terminal output | ||
| addopts = "--cov=docc --cov-report=term-missing" | ||
| testpaths = ["tests"] |
There was a problem hiding this comment.
Yes, we can remove it. I wondered if being implicit and limiting scope to a specific dir provides faster collection, but it doesn't.
|
|
||
| docc.plugins.html = | ||
| templates/** | ||
| templates/*.html |
There was a problem hiding this comment.
It's more explicit. But would fail if we add subdirs. Reverted back to the original (separate commit).
| def test_extension_multiple_suffixes(self, temp_dir: Path) -> None: | ||
| file_path = temp_dir / "archive.tar.gz" | ||
| file_path.write_text("content") | ||
|
|
||
| node = FileNode(file_path) | ||
| assert node.extension == ".gz" |
There was a problem hiding this comment.
I'm not sure this is the behaviour we want. Should we group all extensions?
I haven't really thought about the implications of either behaviour, but we probably should.
There was a problem hiding this comment.
I think the behavior is correct, but the example in the test is poor :) docc won't typically be processing gzipped tarballs.
Absolute path changed from "/path/docs/archive.tar.gz" to "/path/static/chota.min.css".
| @pytest.fixture | ||
| def temp_dir() -> Iterator[Path]: | ||
| with tempfile.TemporaryDirectory() as td: | ||
| yield Path(td) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def basic_settings(temp_dir: Path) -> Settings: | ||
| return Settings(temp_dir, {"tool": {"docc": {}}}) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def plugin_settings(basic_settings: Settings) -> PluginSettings: | ||
| return basic_settings.for_plugin("docc.html") |
There was a problem hiding this comment.
Really need to think of a better way to organize these.
| def test_repr(self) -> None: | ||
| node = TextNode("hello world") | ||
| assert repr(node) == "'hello world'" |
There was a problem hiding this comment.
Tests with special characters, like " or '?
There was a problem hiding this comment.
What determines if a test goes in test_html.py or test_html_comprehensive.py? Could use a docstring or comment explaining what goes where?
| class TestHTMLParserEdgeCases: | ||
| def test_handle_comment_raises(self) -> None: | ||
| context = Context({}) | ||
| parser = HTMLParser(context) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="comments"): | ||
| parser.handle_comment("comment") | ||
|
|
||
| def test_handle_decl_raises(self) -> None: | ||
| context = Context({}) | ||
| parser = HTMLParser(context) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="doctype"): | ||
| parser.handle_decl("DOCTYPE html") | ||
|
|
||
| def test_handle_pi_raises(self) -> None: | ||
| context = Context({}) | ||
| parser = HTMLParser(context) | ||
|
|
||
| with pytest.raises( | ||
| NotImplementedError, match="processing instruction" | ||
| ): | ||
| parser.handle_pi("xml version='1.0'") | ||
|
|
||
| def test_unknown_decl_raises(self) -> None: | ||
| context = Context({}) | ||
| parser = HTMLParser(context) | ||
|
|
||
| with pytest.raises(NotImplementedError, match="unknown"): | ||
| parser.unknown_decl("something") |
There was a problem hiding this comment.
I guess we can assert that these aren't implemented. It'll at least fail and encourage us to write tests if these do get implemented eventually.
| class TestMakeRelativeComprehensive: | ||
| def test_deeply_nested_paths(self) -> None: | ||
| from_path = PurePath("a/b/c/d/e.html") | ||
| to_path = PurePath("a/b/f.html") | ||
| result = _make_relative(from_path, to_path) | ||
|
|
||
| assert result == PurePath("../../f.html") | ||
|
|
||
| def test_different_subtrees(self) -> None: | ||
| from_path = PurePath("root/x/y/z.html") | ||
| to_path = PurePath("root/a/b/c.html") | ||
| result = _make_relative(from_path, to_path) | ||
|
|
||
| assert result == PurePath("../../a/b/c.html") |
There was a problem hiding this comment.
I definitely don't understand why these couldn't have been in test_html.py
There was a problem hiding this comment.
Similar question about what goes where?
| def test_parse_self_closing_tag(self) -> None: | ||
| context = Context({}) | ||
| parser = HTMLParser(context) | ||
| parser.feed("<br>") |
There was a problem hiding this comment.
Worth a test for <br/> as well maybe? I don't know.
| src_listing = next( | ||
| (s for s in sources if s.relative_path == PurePath("src")), None | ||
| ) | ||
| assert ( | ||
| src_listing is not None | ||
| ), "Should create listing for 'src' directory" |
There was a problem hiding this comment.
This is a weird way to express this, IMO. What about something like:
any(s.relative_path == PurePath("src") for s in sources)We already assert on the length earlier, so I think this'll work?
There was a problem hiding this comment.
Found a couple of others, too.
| # The source has output_path="output", so a listing | ||
| # should be created for its parent directory (".") | ||
| # output_path="output" has one parent ".", so 1 listing is created |
There was a problem hiding this comment.
| # The source has output_path="output", so a listing | |
| # should be created for its parent directory (".") | |
| # output_path="output" has one parent ".", so 1 listing is created | |
| # The source has output_path="output", so a single listing | |
| # should be created for its parent directory (".") |
Comment is a bit redundant?
| assert "everyone" in combined | ||
|
|
||
|
|
||
| class TestMarkdownNode: |
There was a problem hiding this comment.
This likely belongs in a test_markdown.py instead, no?
|
|
||
|
|
||
| class TestMarkdownNode: | ||
| def test_children_lazy_loaded(self) -> None: |
There was a problem hiding this comment.
I'm not sure "lazy loaded" is relevant to this test. It doesn't test anything about lazy loading.
| node.replace_child(old_child, new_child) | ||
|
|
||
| new_children = list(node.children) | ||
| assert new_child in new_children |
There was a problem hiding this comment.
| assert new_child in new_children | |
| assert len(new_children) == 1 | |
| assert new_children[0] is new_child |
| f"got: {attr_with_docstring.docstring.text}" | ||
| ) | ||
|
|
||
| def test_determinism(self, temp_dir: Path) -> None: |
There was a problem hiding this comment.
I'm not sure this is a particularly valuable test? Why do we need it?
There was a problem hiding this comment.
Similar comment about clear rules about what kind of tests go in the comprehensive/extended files.
| node.replace_child(old, new) | ||
|
|
||
| new_children = list(node.children) | ||
| assert new in new_children |
| def test_search_children_returns_false(self) -> None: | ||
| node = MarkdownNode(md.Document("test")) | ||
| assert node.search_children() is False | ||
|
|
||
| def test_to_search_returns_text(self) -> None: | ||
| node = MarkdownNode(md.Document("hello world")) | ||
| result = node.to_search() | ||
| assert "hello world" in result |
There was a problem hiding this comment.
Hm, there's some markdown+search stuff in test_integration.py. Where should it live?
| children = list(node.children) | ||
| assert children == [] | ||
|
|
||
| def test_children_lazy_evaluation(self) -> None: |
There was a problem hiding this comment.
I don't think it's important to assert that it is lazily evaluated. That's an implementation detail.
SamWilsn
left a comment
There was a problem hiding this comment.
And this end my first pass over the whole PR.
| """ | ||
| PythonNode.children raises TypeError when a field annotated | ||
| as Node contains a non-Node value. This documents the | ||
| defensive contract in nodes.py:44. |
There was a problem hiding this comment.
Line reference will likely go out of date.
| json_str = output[ | ||
| len("this.SEARCH_INDEX = ") : -len( | ||
| "; Object.freeze(this.SEARCH_INDEX);" | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Should put the extraction code in a function.
|
|
||
| class TestSettingsConstants: | ||
| def test_max_depth(self) -> None: | ||
| assert MAX_DEPTH == 10 | ||
|
|
||
| def test_file_name(self) -> None: | ||
| assert FILE_NAME == "pyproject.toml" |
| def test_line_zero_returns_last_line(self) -> None: | ||
| """ | ||
| line(0) computes lines[0 - 1] = lines[-1], which silently | ||
| returns the last line due to Python negative indexing. | ||
| """ | ||
| content = "first\nsecond\nthird" | ||
| source = ConcreteTextSource(content) | ||
| # line(0) accesses lines[-1] which is "third" | ||
| assert source.line(0) == "third" |
There was a problem hiding this comment.
This is probably a bug, not something we should enforce?
There was a problem hiding this comment.
Lol, I'll delete these tests. I think this is unlikely to be triggered, but can add a guard to fix it in a follow-up if you like?
Add tests for TextNode repr with double quotes and single quotes, verifying Python's repr behavior with these characters. Addresses: SamWilsn#37 (comment)
Strengthen `test_append_child` and `test_append_text` to verify the appended node is the last child, not just present somewhere. Addresses: SamWilsn#37 (comment)
Strengthen `test_replace_child` to verify the new node takes the exact index position of the old node, not just that it's present. Addresses: SamWilsn#37 (comment)
Test that when the same child is appended twice, `replace_child` replaces both occurrences with the new node. Addresses: SamWilsn#37 (comment)
Verify that the XHTML-style `<br/>` is parsed identically to `<br>`. Addresses: SamWilsn#37 (comment)
Replace `test_html.py`, `test_html_comprehensive.py`, and `test_html_extended.py` with four clearly-scoped modules: - `test_html_nodes.py` — `TextNode`, `HTMLTag`, `HTMLRoot`, `_to_element`. - `test_html_parser.py` — `HTMLParser`, `_ElementTreeVisitor`, `_make_relative`. - `test_html_render.py` — render callbacks, `HTMLVisitor`, `_FindVisitor`, `render_reference`. - `test_html_plugin.py` — `HTMLContext`, `HTMLDiscover`, `HTMLTransform`, `HTMLRoot.output`. Drop test classes in favour of flat `test_` functions (no shared parametrisation or fixtures justified classes). Deduplicate 5 tests that appeared identically across the old files (105 → 100). Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
Strengthen `MarkdownNode.replace_child` test to verify the children list has exactly one element and it is the replacement node. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Remove `test_children_lazy_evaluation` — whether `MarkdownNode` caches children internally is an implementation detail, not a behavioral contract worth testing. Addresses: SamWilsn#37 (comment)
Replace `test_mistletoe_comprehensive.py` and `test_mistletoe_extended.py` with two clearly-scoped modules: - `test_mistletoe.py` — `MarkdownNode`, visitors (`_DocstringVisitor`, `_ReferenceVisitor`, `_SearchVisitor`), and transforms. - `test_mistletoe_render.py` — all `_render_*` functions and `render_html` dispatch. Move `TestMarkdownNode` and `TestSearchVisitor` from `test_integration.py` into `test_mistletoe.py` where they belong. Drop test classes in favour of flat `test_` functions. Deduplicate tests that appeared across files (80 → 76). Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
The determinism test runs the same pipeline twice and compares output, but this doesn't test meaningful behaviour — the pipeline is already deterministic by construction. Addresses: SamWilsn#37 (comment)
17b9061 to
1430972
Compare
Remove tests that just pin `MAX_DEPTH == 10` and `FILE_NAME == "pyproject.toml"` — these assert constant values with no behavioural insight. Addresses: SamWilsn#37 (comment)
Convert 22 single-method test classes across 10 files into top-level `test_` functions. Classes with multiple methods are left as-is. Renamed functions to be self-descriptive without the class namespace, for example `TestBuilderContextManager::test_builder_with_statement` becomes `test_builder_context_manager`. Addresses: SamWilsn#37 (comment)
Replace all `@pytest.fixture def temp_dir()` using `tempfile.TemporaryDirectory` with pytest's built-in `tmp_path` fixture across 11 test files. Also convert inline `tempfile.TemporaryDirectory()` usage in `test_context.py`. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
Add parametrized tests for `children` iterability and `repr` across `BlankNode` and `ListNode`, making it easy to extend when new node types are added. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Replace all manual `os.chdir()` + try/finally blocks with pytest's `monkeypatch.chdir()` which automatically restores the working directory. This is safer for parallel test execution. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
- Assert "Output path is required" in log when `main()` exits without an output path, verifying the _reason_ for failure. - Add return type to `ContainerNode.children`. - Raise `NotImplementedError` in `ContainerNode.replace_child`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
Use `ListNode([first, second]).visit(visitor)` instead of manually calling `enter`/`exit`, making the test a fairer integration check. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Verify the end-to-end pipeline actually renders the docstring into the output HTML, not just that files are created. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Verify that storing instances under both `Base` and `Derived` keys resolves each independently via exact type matching. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Test that `.tar.gz` removes only the final suffix (`.gz`), leaving `.tar` — documenting the current single-suffix-stripping behaviour. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Check `source in processed` first (did it get processed?), then `len(processed) == 1` (nothing unexpected?), then `len(unprocessed)`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Replace 5 duplicated JSON extraction blocks with a shared `_parse_search_output()` function that validates the prefix/suffix and parses the payload. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Simplify existence checks to use `any(s.relative_path == ...)`. Where the result is used afterward, drop the `None` sentinel and let `StopIteration` propagate on failure. Applied to all 3 occurrences in `test_listing.py`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Remove `nodes.py:44` reference that will go out of date. No other line references remain in test files. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Remove the third line that restates what the first two already say. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment)
Replace string-based event tracking (`f"enter:{repr(node)}"`) with
`(action, id(node))` tuples so assertions verify exact object
identity, not just repr equality.
Make `SkippingVisitor` and `ConditionalSkipVisitor` inherit from
`RecordingVisitor` via a `ClassVar` return-value override, removing
duplicated enter/exit logic.
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
Addresses: SamWilsn#37 (comment)
Addresses: SamWilsn#37 (comment)
pytest discovers `tests/` via normal collection without needing `testpaths` — it's the default behaviour. Addresses: SamWilsn#37 (comment)
All test files were written in 2026 and should reflect the current year. Addresses: SamWilsn#37 (comment)
Remove `test_discover_yields_source`, `test_builder_processes_source`, and `test_builder_context_manager` — these only tested `ConcreteDiscover`/`ConcreteBuilder` defined in the test file, not production code. Real discover/builder subclasses are already tested in `test_python_cst.py`, `test_files.py`, `test_listing.py`, etc. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment)
Replace `archive.tar.gz` with `chota.min.css` — a file docc actually ships — making the compound extension examples more realistic. Addresses: SamWilsn#37 (comment)
The explicit glob patterns were an unnecessary change — the original `templates/**` works correctly for both sdist and wheel builds. Addresses: SamWilsn#37 (comment)
Remove `TestTextSourceBoundary` which asserted that `line(0)` and `line(-1)` silently return wrong lines via Python negative indexing. This is a bug in production code, not behaviour to enforce in tests. Addresses: SamWilsn#37 (comment)
|
Hey @SamWilsn, thank you so much for your careful review on this PR. Absolutely incredible, I would have auto-closed it myself. I think I've addressed your comments. Some of them were no brainers, I'm sorry they reached you. Claude and I organized our work as follows: We tried to do the large breaking changes first and then the smaller things last. This was probably the wrong way around. At the bottom of each commit message, I put the link to your comment(s) that the commit targets. I'm not sure if it makes it faster, but you can re-review/verify all comments are addressed by going commit-by-commit and clicking on the link for context and then resolve your comment if so. |
| def test_transform_list_node(tmp_path: Path) -> None: | ||
| settings = Settings(tmp_path, {"tool": {"docc": {}}}) | ||
| plugin_settings = settings.for_plugin("docc.html.transform") | ||
|
|
||
| node = ListNode([BlankNode(), BlankNode()]) | ||
| document = Document(node) | ||
| context = Context({Document: document}) | ||
|
|
||
| transform = HTMLTransform(plugin_settings) | ||
| transform.transform(context) | ||
|
|
||
| assert isinstance(document.root, HTMLRoot) | ||
| assert document.root.extension == ".html" |
There was a problem hiding this comment.
This seems suspiciously similar to test_html_e2e.test_list_with_blanks_becomes_html_root. Do we need both?
| def test_output_breadcrumbs_for_nested_path() -> None: | ||
| source = MockSource(PurePath("a/b/page.html")) | ||
| context = Context({Source: source}) | ||
| root = HTMLRoot(context) | ||
| root.append(HTMLTag("p")) | ||
|
|
||
| dest = StringIO() | ||
| root.output(context, dest) | ||
| output = dest.getvalue() | ||
|
|
||
| assert "breadcrumbs" in output | ||
| assert "page.html" in output |
There was a problem hiding this comment.
Similarly test_html_e2e.test_output_breadcrumbs_for_nested_path
| # Verify the tree contains Module, Class, Function nodes | ||
| class NodeTypeChecker(Visitor): | ||
| found_module = False | ||
| found_class = False | ||
| found_function = False | ||
|
|
||
| def enter(self, node: Node) -> Visit: | ||
| if isinstance(node, nodes.Module): | ||
| self.found_module = True | ||
| elif isinstance(node, nodes.Class): | ||
| self.found_class = True | ||
| elif isinstance(node, nodes.Function): | ||
| self.found_function = True | ||
| return Visit.TraverseChildren | ||
|
|
||
| def exit(self, node: Node) -> None: | ||
| pass | ||
|
|
||
| checker = NodeTypeChecker() | ||
| document.root.visit(checker) | ||
| assert checker.found_module, "Should contain a Module node" | ||
| assert checker.found_class, "Should contain a Class node" | ||
| assert checker.found_function, "Should contain a Function" |
There was a problem hiding this comment.
This seems redundant. The below checks will fail if the module/class/function aren't present. Is there value in checking twice?
Fix glob patterns in template packaging - Use explicit patterns instead of recursive ** globs. - Setuptools doesn't reliably include files with ** in sdist builds. Add test extras with pytest-cov dependency - Add dedicated test extras with pytest and pytest-cov. Include test extras in tox environments - Add test extras to both test and type environments so pytest and pytest-cov are available alongside lint deps. Add pytest and coverage configuration - Configure pytest to run with coverage enabled. - Set minimum coverage threshold to 80%. - Exclude common non-testable patterns from coverage. Add development section to README Add comprehensive test suite - Add tests for CLI, settings, context, and document modules. - Add tests for plugin loader and transform pipeline. - Add tests for HTML, mistletoe, and verbatim plugins. - Add end-to-end HTML rendering pipeline tests. - Add tests for Python CST parsing and node types. - Add tests for references, search, and resources plugins. - Add integration tests for end-to-end workflows. - Add behavior-level pipeline contract tests. - Achieve 90% code coverage. test(html): add TextNode repr tests for special characters Add tests for TextNode repr with double quotes and single quotes, verifying Python's repr behavior with these characters. Addresses: SamWilsn#37 (comment) test(html): assert `append` puts child at end of children list Strengthen `test_append_child` and `test_append_text` to verify the appended node is the last child, not just present somewhere. Addresses: SamWilsn#37 (comment) test(html): assert `replace_child` preserves position of old node Strengthen `test_replace_child` to verify the new node takes the exact index position of the old node, not just that it's present. Addresses: SamWilsn#37 (comment) test(html): add `replace_child` test with duplicate children Test that when the same child is appended twice, `replace_child` replaces both occurrences with the new node. Addresses: SamWilsn#37 (comment) test(html): add parser test for `<br/>` self-closing syntax Verify that the XHTML-style `<br/>` is parsed identically to `<br>`. Addresses: SamWilsn#37 (comment) test(html): reorganize into four focused test modules Replace `test_html.py`, `test_html_comprehensive.py`, and `test_html_extended.py` with four clearly-scoped modules: - `test_html_nodes.py` — `TextNode`, `HTMLTag`, `HTMLRoot`, `_to_element`. - `test_html_parser.py` — `HTMLParser`, `_ElementTreeVisitor`, `_make_relative`. - `test_html_render.py` — render callbacks, `HTMLVisitor`, `_FindVisitor`, `render_reference`. - `test_html_plugin.py` — `HTMLContext`, `HTMLDiscover`, `HTMLTransform`, `HTMLRoot.output`. Drop test classes in favour of flat `test_` functions (no shared parametrisation or fixtures justified classes). Deduplicate 5 tests that appeared identically across the old files (105 → 100). Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(mistletoe): assert on length and order after `replace_child` Strengthen `MarkdownNode.replace_child` test to verify the children list has exactly one element and it is the replacement node. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(mistletoe): remove lazy evaluation assertion Remove `test_children_lazy_evaluation` — whether `MarkdownNode` caches children internally is an implementation detail, not a behavioral contract worth testing. Addresses: SamWilsn#37 (comment) test(mistletoe): reorganize into two focused test modules Replace `test_mistletoe_comprehensive.py` and `test_mistletoe_extended.py` with two clearly-scoped modules: - `test_mistletoe.py` — `MarkdownNode`, visitors (`_DocstringVisitor`, `_ReferenceVisitor`, `_SearchVisitor`), and transforms. - `test_mistletoe_render.py` — all `_render_*` functions and `render_html` dispatch. Move `TestMarkdownNode` and `TestSearchVisitor` from `test_integration.py` into `test_mistletoe.py` where they belong. Drop test classes in favour of flat `test_` functions. Deduplicate tests that appeared across files (80 → 76). Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(integration): remove `test_determinism` The determinism test runs the same pipeline twice and compares output, but this doesn't test meaningful behaviour — the pipeline is already deterministic by construction. Addresses: SamWilsn#37 (comment) test(settings): remove `TestSettingsConstants` Remove tests that just pin `MAX_DEPTH == 10` and `FILE_NAME == "pyproject.toml"` — these assert constant values with no behavioural insight. Addresses: SamWilsn#37 (comment) test: unwrap single-method test classes into bare functions Convert 22 single-method test classes across 10 files into top-level `test_` functions. Classes with multiple methods are left as-is. Renamed functions to be self-descriptive without the class namespace, for example `TestBuilderContextManager::test_builder_with_statement` becomes `test_builder_context_manager`. Addresses: SamWilsn#37 (comment) test: replace hand-rolled `temp_dir` fixtures with `tmp_path` Replace all `@pytest.fixture def temp_dir()` using `tempfile.TemporaryDirectory` with pytest's built-in `tmp_path` fixture across 11 test files. Also convert inline `tempfile.TemporaryDirectory()` usage in `test_context.py`. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(document): parametrize base node operations Add parametrized tests for `children` iterability and `repr` across `BlankNode` and `ListNode`, making it easy to extend when new node types are added. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(cli): replace `os.chdir` with `monkeypatch.chdir` Replace all manual `os.chdir()` + try/finally blocks with pytest's `monkeypatch.chdir()` which automatically restores the working directory. This is safer for parallel test execution. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(cli): strengthen assertions and fix mock type annotations - Assert "Output path is required" in log when `main()` exits without an output path, verifying the _reason_ for failure. - Add return type to `ContainerNode.children`. - Raise `NotImplementedError` in `ContainerNode.replace_child`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(cli): use `ListNode` for multiple output node traversal Use `ListNode([first, second]).visit(visitor)` instead of manually calling `enter`/`exit`, making the test a fairer integration check. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(cli): assert "Module docstring" appears in rendered output Verify the end-to-end pipeline actually renders the docstring into the output HTML, not just that files are created. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(context): add test storing both `Base` and `Derived` Verify that storing instances under both `Base` and `Derived` keys resolves each independently via exact type matching. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(files): add compound extension test for `FileSource.output_path` Test that `.tar.gz` removes only the final suffix (`.gz`), leaving `.tar` — documenting the current single-suffix-stripping behaviour. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(files): reorder builder assertions for clarity Check `source in processed` first (did it get processed?), then `len(processed) == 1` (nothing unexpected?), then `len(unprocessed)`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(search): extract `_parse_search_output` helper Replace 5 duplicated JSON extraction blocks with a shared `_parse_search_output()` function that validates the prefix/suffix and parses the payload. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(listing): use `any()` instead of `next(..., None)` pattern Simplify existence checks to use `any(s.relative_path == ...)`. Where the result is used afterward, drop the `None` sentinel and let `StopIteration` propagate on failure. Applied to all 3 occurrences in `test_listing.py`. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(python_cst): remove line reference from docstring Remove `nodes.py:44` reference that will go out of date. No other line references remain in test files. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(listing): trim redundant comment Remove the third line that restates what the first two already say. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) test(document): refactor visitor helpers to use `id()` identity Replace string-based event tracking (`f"enter:{repr(node)}"`) with `(action, id(node))` tuples so assertions verify exact object identity, not just repr equality. Make `SkippingVisitor` and `ConditionalSkipVisitor` inherit from `RecordingVisitor` via a `ClassVar` return-value override, removing duplicated enter/exit logic. Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com> Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test: remove redundant `testpaths` from pytest config pytest discovers `tests/` via normal collection without needing `testpaths` — it's the default behaviour. Addresses: SamWilsn#37 (comment) test: update copyright year to 2026 in all test files All test files were written in 2026 and should reflect the current year. Addresses: SamWilsn#37 (comment) test(build): remove tests that only exercise test-local mocks Remove `test_discover_yields_source`, `test_builder_processes_source`, and `test_builder_context_manager` — these only tested `ConcreteDiscover`/`ConcreteBuilder` defined in the test file, not production code. Real discover/builder subclasses are already tested in `test_python_cst.py`, `test_files.py`, `test_listing.py`, etc. Addresses: SamWilsn#37 (comment) Addresses: SamWilsn#37 (comment) test(files): use `chota.min.css` for compound extension tests Replace `archive.tar.gz` with `chota.min.css` — a file docc actually ships — making the compound extension examples more realistic. Addresses: SamWilsn#37 (comment) revert: restore original `templates/**` globs in setup.cfg The explicit glob patterns were an unnecessary change — the original `templates/**` works correctly for both sdist and wheel builds. Addresses: SamWilsn#37 (comment) test(source): remove tests that pin buggy negative-index behaviour Remove `TestTextSourceBoundary` which asserted that `line(0)` and `line(-1)` silently return wrong lines via Python negative indexing. This is a bug in production code, not behaviour to enforce in tests. Addresses: SamWilsn#37 (comment) Update conftest.py Update test_mistletoe.py
3226a97 to
510cdf1
Compare
Uh oh!
There was an error while loading. Please reload this page.