Skip to content

test(docs): add unit tests for MDX helpers in create_docs.py (HYBIM-725)#63

Open
etserend wants to merge 4 commits into
mainfrom
test/create-docs-mdx-helpers
Open

test(docs): add unit tests for MDX helpers in create_docs.py (HYBIM-725)#63
etserend wants to merge 4 commits into
mainfrom
test/create-docs-mdx-helpers

Conversation

@etserend

@etserend etserend commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds `tests/test_create_docs_helpers.py` with 22 unit tests covering the three MDX helper functions added in HYBIM-725:
    • `_escape_curly_braces` — braces outside fenced/inline code are escaped for MDX
    • `_convert_rst_code_blocks` — RST `::` indented blocks converted to fenced ```python`
    • `_sanitize_description` — composition of the two above
  • Adds `docstring-parser` to test dependencies so helpers are imported directly from `scripts/create_docs.py`
  • Includes edge case tests for `::` mid-line (e.g. `namespace::method`) and prose lines ending in `::` with no indented block

Test plan

  • CI green (all Python versions / OS matrix) ✅
  • All 22 tests pass locally ✅

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.

Verdict: approve — Tests are correct and exercise the real helpers; only a stale PR description and a lint nit remain.

General Comments

  • 🟡 minor (documentation): The PR description states: "Functions are inlined in the test file (not imported from the script) to avoid a docstring_parser dependency that is not part of the project's dev dependencies." The implementation does the opposite — tests/test_create_docs_helpers.py imports the helpers directly from create_docs (line 14), and the PR adds docstring-parser to the test dependency group in pyproject.toml (with the corresponding poetry.lock change moving it into the test group). Importing the real functions is the better choice (the tests will actually catch regressions in the source), but please update the description so it matches the code — as written it would mislead a reviewer about the design.

Follow-ups

Suggested follow-up work that could be tracked as Shortcut stories:

  • scripts/create_docs.py:522-550: _convert_rst_code_blocks treats any line whose rstripped text ends with :: as the start of a code block. Prose ending in a double colon (or a trailing :: from something like a namespace reference) would be misinterpreted and stripped. The new tests don't cover this ambiguity. Consider adding a test (and, if it turns out to be a real defect, a guard in the source) for a non-code line ending in ::.

Comment thread tests/test_create_docs_helpers.py
fercor-cisco
fercor-cisco previously approved these changes Jul 7, 2026

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.

Verdict: request_changes — Test logic is correct and matches the implementation, but the new file has lint violations (E402 already flagged, plus an unnoticed import-order I001) that will fail the ruff pre-commit hook despite the "CI green" claim.

Follow-ups

Suggested follow-up work that could be tracked as Shortcut stories:

  • tests/test_create_docs_helpers.py:1-162: Coverage gaps in the MDX helpers not exercised here (non-blocking): (1) _escape_curly_braces with an unclosed/odd triple-backtick fence — the re.split(r"(```.*?```)") regex leaves a dangling fence in a normal (escaped) segment; (2) inline code spans with unbalanced backticks; (3) _convert_rst_code_blocks with tab-indented (rather than 4-space) RST blocks, since the collector only recognizes lines starting with exactly four spaces; (4) a :: block whose content is indented more than 4 spaces (verifying only the first 4 are stripped). Adding these would harden the helpers against real-world docstrings.

Comment on lines +9 to +14
import sys
import os

sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts"))

from create_docs import _escape_curly_braces, _convert_rst_code_blocks, _sanitize_description

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 minor (other): Two lint issues in this import block that will fail the ruff pre-commit hook (contradicting the "CI green" claim):

  1. E402 (already flagged in an existing unresolved review): the from create_docs import ... after sys.path.insert(...) is a module-level import not at top of file. E4 is selected in pyproject.toml and E402 is not in the tests/**/*.py ignore list. It is not auto-fixable, so the hook fails. tests/conftest.py handles the same pattern with # noqa: E402.

  2. I001 (import sorting): import sys precedes import os, but isort (I is selected, not ignored for tests) wants alphabetical order (os before sys). This is auto-fixable, but the hook will still report a modification.

Suggested change
import sys
import os
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts"))
from create_docs import _escape_curly_braces, _convert_rst_code_blocks, _sanitize_description
import os
import sys
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts"))
from create_docs import _escape_curly_braces, _convert_rst_code_blocks, _sanitize_description # noqa: E402

🤖 Generated by Astra

Comment thread poetry.lock

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 minor (other): This lock file was regenerated with a newer Poetry (2.2.1 → 2.4.1), which rewrote environment markers across many packages unrelated to the intended change (charset-normalizer, distro, jiter, jsonpatch, langchain-core, openai, pyyaml, requests, tenacity, urllib3, uuid_utils, zstandard, etc.). The only intended lock change for this PR is promoting docstring-parser into the test group. Consider regenerating the lock with the same Poetry version the repo standardizes on so the diff is limited to docstring-parser; the unrelated marker churn creates merge friction and makes it hard to see the real change. Please confirm these marker rewrites are benign and expected.

🤖 Generated by Astra

etserend added 3 commits July 7, 2026 13:45
… (HYBIM-725)

Covers _escape_curly_braces, _convert_rst_code_blocks, and _sanitize_description —
the helpers added to fix prose-only example fencing and MDX brace escaping.
…er as dev dep (HYBIM-725)

Add docstring-parser to test dependencies so helpers can be imported directly
from scripts/create_docs.py instead of duplicated. Add 4 new test cases:
adjacent fenced blocks, mixed inline/prose braces, prose-after-block, and
sanitize ordering. Add class/method docstrings to match project test style.
Documents behavior when :: appears mid-line (namespace::method) and when
a prose line ends with :: but has no indented block following.
@etserend etserend force-pushed the test/create-docs-mdx-helpers branch from ef1b11f to 8351e1d Compare July 7, 2026 18:46
@etserend

etserend commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main after PR #66 merged (ruff bump changed pyproject.toml and poetry.lock). No functional changes — docstring-parser was already added to the lockfile as part of #66, so the rebase was clean.

…dded to test group

poetry.lock was stale — the >=0.17.0 constraint in [test] deps conflicted with
the >=0.16,<1.0 metadata from the lockfile inherited from main.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@etserend etserend requested a review from fercor-cisco July 7, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants