Skip to content

test: decide and wire a testing strategy for grimoire's executable code (#34) - #53

Open
Jartans-Familiar wants to merge 2 commits into
mainfrom
agent/loom/1ea9712f-testing
Open

test: decide and wire a testing strategy for grimoire's executable code (#34)#53
Jartans-Familiar wants to merge 2 commits into
mainfrom
agent/loom/1ea9712f-testing

Conversation

@Jartans-Familiar

Copy link
Copy Markdown
Member

Closes #34.

Stacked on #51 (agent/loom/1ea9712f) -- the Node test file lives beside
find-duplicate-comments.js at its post-split location
(plugins/praxis/skills/comment-hygiene/), so this PR's base is that
branch rather than main. Merge #51 first; this one will retarget main
automatically once it does.

Decision (the questions #34 asked)

  • Node: node:test + node:assert/strict. Ships with the Node already
    running every hook -- zero new dependency, no package.json/lockfile to
    add. Tests colocate with source as *.test.js, node:test's own
    zero-config discovery pattern (node --test with no path args finds them
    recursively from repo root).
  • Python: stdlib unittest, not pytest -- the same reasoning
    .pre-commit-config.yaml already states for skipping ruff: a pyproject.toml
    to hold config that nothing else in the repo needs. Tests live in scripts/
    as test_*.py (unittest discover's default pattern) and load
    generate-codex.py via importlib.util, since its hyphenated filename
    can't be a plain import.
  • One strategy or two: two, deliberately -- each stays idiomatic to its
    own runtime's zero-config discovery rather than fighting either tool to
    force a shared filename convention across languages.
  • Scope: functions that carry real parsing or bookkeeping logic.
    find-duplicate-comments.js's exported pure functions (prose,
    buildSkipMatcher, findRetoldInDiff, dedupePairs -- seamed for this in
    feat: repo hygiene enforcement, plus praxis compact and skill-activation fixes #33, orphaned since, per Decide a testing strategy for the marketplace's executable code #34's own description) get 19 cases covering the
    three defect classes feat: repo hygiene enforcement, plus praxis compact and skill-activation fixes #33's review rounds caught by hand: the diff
    line-number off-by-one, the unescaped---skip-pattern false-clean, and the
    CRLF false-clean. generate-codex.py's parse_frontmatter, qualify,
    display_name, short_description, basic_string (the TOML escaper) and
    render_agent_toml get 21 cases.
    Out of scope, named rather than silently dropped: the thin hook
    wrappers (check-console-log.js, suggest-compact.js, suggest-skills.js,
    sync-codex-agents.js) -- each is mostly a call into the Claude Code hook
    API, where a fixture would assert little; and generate-codex.py's
    filesystem-walking functions (collect_targets, find_orphans,
    find_divergent_copies, main), which already run end to end against the
    real repo on every make verify --check -- that IS their regression
    coverage, and duplicating it under a mock filesystem would test the mock,
    not the logic.
  • Wiring: make test, a new test job in ci.yml (needs
    actions/setup-node alongside the existing setup-python), and both the
    needs: list and the results array of the check aggregator, per the
    hand-sync note that file already carries. CLAUDE.md, CONTRIBUTING.md
    and README.md all updated so make test is part of the stated
    pre-PR/pre-done checklist, not a step only CI knows about.

Demonstrated: red before, green after

Both done locally against this branch, then reverted before committing --
neither seeded defect is in the diff:

Node -- removed the "no newline at end of file" marker guard in
findRetoldInDiff (the exact historical off-by-one #34 names):

✖ keeps line numbers correct across a "no newline at end of file" marker
  AssertionError: Expected values to be strictly equal:
  + 'new/file.js:4'
  - 'new/file.js:3'

Restored -> node --test: 19/19 pass.

Python -- swapped basic_string's escape order (quote before backslash
instead of after), which lets a TOML parser read the output as ending the
string early rather than as an escaped quote:

FAIL: test_escaping_a_backslash_and_a_quote_together_stays_parseable
AssertionError: '"a\\\\\\\\"b"' != '"a\\\\\\"b"'

Restored -> python3 -m unittest discover: 21/21 pass.

Domain-owner clause

Artifact-only. This is test infrastructure and CI wiring for code Loom
already owns test coverage for per the role spec; no skill or agent
content changes.

Manifest / Codex face

praxis bumped 1.5.0 -> 1.5.1 (PATCH: the new test file is
non-behavioural -- it ships as plugin content because it's colocated under
skills/comment-hygiene/, but nothing at runtime reads it). Codex face:
general-reviewer.toml and the plugin manifests regenerated by
scripts/generate-codex.py; nothing under plugins/praxis/agents/ or
codex/ changed content-wise in this PR, so the regen diff is the version
line only.

Verification

make verify   # clean
make test     # 19 Node + 21 Python, all pass
make lint     # clean (lychee-docker/actionlint-docker skip in this sandbox --
              # no Docker socket access; codespell/shellcheck/markdownlint-cli2/
              # zizmor/pre-commit-hooks all pass)

Task JAR-413.

Jartans-Familiar and others added 2 commits August 17, 2026 10:01
code-hygiene bundled comment truthfulness with dead code, reinvention,
naming and faking-done, so a diff that only needed the comment rules
did not reliably auto-activate the skill (#39). Comments, tombstone
and retold-fact detection, and comment density now live in their own
comment-hygiene skill; find-duplicate-comments.js moves with them.

Cross-references updated across code-structure, readable-code,
docs-patterns and general-reviewer (frontmatter skills: list plus the
Important/Minor bullets that were comment-specific). Content is moved,
not rewritten -- artifact-only, no new doctrine.

praxis bumped 1.4.0 -> 1.5.0 (new skill, minor per RELEASING.md).
Codex face: neither .claude-plugin/plugin.json nor .codex-plugin
enumerates skills, so no manifest entry to add; general-reviewer.toml
regenerated because its source .md body and skills: list changed.

Refs JAR-413

Co-authored-by: multica-agent <github@multica.ai>
…de (#34)

grimoire's Node hooks/reviewer tooling and scripts/generate-codex.py ran
zero assertions; defects were caught only by hand during review (#33's
review rounds: an off-by-one in diff line bookkeeping, a path-quoting
false-clean, a CRLF false-clean, a submodule abort). Decision, per #34's
open questions:

- Node: node:test + node:assert/strict. Ships with the Node already
  running every hook -- no new dependency, no package.json. Tests live
  beside their source as *.test.js, node:test's own zero-config discovery.
- Python: stdlib unittest, not pytest -- same reasoning
  .pre-commit-config.yaml already gives for skipping ruff (a pyproject.toml
  to hold config that nothing else in the repo needs). Tests live in
  scripts/ as test_*.py (unittest's discovery default) and load
  generate-codex.py via importlib, since its hyphenated filename blocks a
  plain import.
- Two conventions, not one: each stays idiomatic to its own runtime's
  zero-config discovery rather than fighting either tool to force a shared
  pattern.
- Scope: functions carrying real parsing or bookkeeping logic --
  find-duplicate-comments.js's exported pure functions (already seamed for
  this in #33 and orphaned since) and generate-codex.py's frontmatter
  parser, qualify/display_name/short_description, and the TOML string
  escaper. Thin hook wrappers that only call back into the Claude Code
  hook API are out of scope -- a fixture would assert little. The
  filesystem-walking functions (collect_targets, find_orphans,
  find_divergent_copies) run end to end against the real repo on every
  `make verify --check`, which is itself their regression coverage.
- Wiring: `make test`, a new `test` CI job (needs actions/setup-node
  alongside setup-python), and both the needs list and the results array
  of ci.yml's check aggregator per its own hand-sync note.

Demonstrated locally (not committed, restored before this commit): reintroducing
the historical no-newline-marker off-by-one in findRetoldInDiff fails
`keeps line numbers correct across a "no newline at end of file" marker`;
swapping basic_string's escape order fails
`test_escaping_a_backslash_and_a_quote_together_stays_parseable`. Both red
before the revert, both green after.

praxis bumped 1.5.0 -> 1.5.1 (PATCH: the new test file is non-behavioural,
co-located under a skill directory that already ships as plugin content).
Codex face: general-reviewer.toml etc. regenerated, no content changed
under them so the diff is the version line only.

Refs JAR-413

Co-authored-by: multica-agent <github@multica.ai>
@Jartans-Familiar

Copy link
Copy Markdown
Member Author

Holding this one. #34 was decided and closed by #50, merged as ee341bd, and the convention now lives in CONTRIBUTING.md under ## Tests. This PR re-answers the same question and lands on the other side of it in four places, so it cannot merge as written:

  1. Test location. This colocates (plugins/praxis/skills/comment-hygiene/find-duplicate-comments.test.js, scripts/test_generate_codex.py). The merged convention puts tests in tests/, for two repo-specific reasons: everything under plugins/<name>/ ships to whoever installs that plugin and a test suite is not part of the offer, and a content change there requires a version bump in its plugin.json -- so a colocated test forces a plugin release on every test edit.
  2. Discovery. This relies on node --test with no path arguments and unittest discover. The merged convention requires explicit git ls-files lists: node --test reads a directory argument on Node 18 and rejects it on Node 24, and takes a ** glob on Node 24 and rejects it on Node 18, so a file list is the only form that works on both -- which matters because a contributor's Node and CI's Node differ. unittest discover separately wants an __init__.py in every test directory.
  3. Hook entry points. Named out of scope here as thin wrappers. The merged convention makes a process contract per shipped hook entry point required (class 2): a hook that throws breaks the user's turn, and no other check in this repository can see it. Those four contracts are in JAR-430: cover the hook scripts and their process contracts #54.
  4. The drift finders. find_orphans and find_divergent_copies are out of scope here on the grounds that make verify exercises them. It exercises them against one tree state -- the current, clean one -- which is the state in which they return nothing. Neither the orphan case nor the divergence case is reached, so that is not regression coverage for either. Both are class 1 in the merged convention and are covered in JAR-430: wire make test and cover the two parsing-heavy files #52.

Also: Closes #34 will not do anything, since #50 already closed it.

The gate itself collides outright -- both branches define make test and a test job plus the check aggregator edit, on the same lines.

#52 and #54 implement the merged convention: 139 Node tests and 40 Python tests, the four hook process contracts, and the four find-duplicate-comments.js regressions each confirmed to fail with its fix reverted.

What is worth keeping from here is the coverage, not the wiring. If any of the 40 cases in this branch assert something #52 misses, port those cases onto tests/ and close this. Otherwise close it. The decision is not reopenable from a PR -- if the location or discovery rule is wrong, that is an amendment to CONTRIBUTING.md argued on its own.

Base automatically changed from agent/loom/1ea9712f to main August 17, 2026 09:10
@JartanFTW

Copy link
Copy Markdown
Contributor

resolve conflicts

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.

Decide a testing strategy for the marketplace's executable code

2 participants