test(cli): add CLI-level coverage for log and timeline option parsing - #46
test(cli): add CLI-level coverage for log and timeline option parsing#46mvanhorn wants to merge 3 commits into
Conversation
Closes mex-memory#43. Adds direct tests for the Commander wiring of mex log / mex timeline so the option-parsing risks called out in the PR mex-memory#39 review have explicit regression coverage: - parseIntArg / parsePositiveIntArg are now exported and unit-tested for the accept and reject paths (non-negative integers, positive integers, negative input, non-numeric input, zero). - program is exported and parsed via program.parseAsync so the test exercises the same wiring path the production binary uses. findConfig and the events runners are mocked so the tests don't touch disk. - mex log: covers single --file pass-through, repeated --file accumulator order, default --type='note', and the action's error-propagation path when runLog rejects. - mex timeline: covers --limit parsed via parsePositiveIntArg, plus --json / --since / --type / --limit thread-through. Auto-running program.parse() is now gated on this module being the main entrypoint so test imports don't trip Commander on test runner argv.
theDakshJaitly
left a comment
There was a problem hiding this comment.
Thanks Matt — tests pass locally (13/13), build is clean, and dist/cli.js still rejects bad --limit the same as before. The coverage gap from #39 is closed nicely. Two small things to fix before merge:
1. Main-module guard isn't portable (src/cli.ts:303-310)
import.meta.url ===
(typeof process !== "undefined" && process.argv[1]
? new URL(`file://${process.argv[1]}`).href
: undefined)Hand-building file://${process.argv[1]} breaks on Windows paths (C:\…) and any path with spaces or unicode — no percent-encoding. Node ships the canonical helper, and engines.node >= 20 means it's always available:
import { pathToFileURL } from "node:url";
// …
if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
program.parse();
}macOS/Linux with ASCII paths happens to work today, but this'll bite the first Windows user or anyone npm link-ing from a path with a space.
2. Unused import (test/cli.test.ts:3)
import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs";writeFileSync is never referenced — drop it.
Optional nits (non-blocking)
- The
tmpDir/.mex/eventsscaffold inbeforeEachis dead setup since every test mocksrunLog/runTimeline. Could shrink to just themexConfigobject. programis a Commander singleton with accumulating state acrossparseAsynccalls — fine here since each test hits a distinct subcommand, but worth keeping in mind as this file grows.- Test "propagates an invalid --type" really exercises the generic action-handler error path, not anything
--type-specific. Title is slightly misleading but the assertion is fine.
|
Addressed both requested changes: 1. Portable main-module guard (
2. Unused import (
Verified locally: Commit: https://github.com/advancedresearcharray/mex/commit/3f6e17d (branch @mvanhorn — ready to cherry-pick or merge that commit onto this PR branch when convenient. |
|
Addressed the review feedback from @theDakshJaitly — with one important context update: Test coverage already merged via #47. The CLI-level Remaining review item (portable main-module guard) is in a rebased follow-up: #67 — #67 Changes in #67 (on current
The unused Verified locally: Could not push directly to this branch (403 to |
|
Both blocking review items are implemented on branch Commit: https://github.com/advancedresearcharray/mex/commit/0956383
Verified: Could not push directly to Note: CLI test coverage from this PR already landed on |
|
Updated fix commit (no co-author trailer): https://github.com/advancedresearcharray/mex/commit/7bf6508 — mvanhorn/mex#3 tracks the same branch for merging onto this PR's head. |
Addresses review on mex-memory#46: replace hand-built file:// URL with Node's pathToFileURL helper for Windows/unicode path safety, and drop unused writeFileSync import from test/cli.test.ts.
|
Both blocking review items are implemented and verified on
Could not push to
Once the head branch includes |
…ing-43 fix(cli): address PR mex-memory#46 review feedback
|
Merged @advancedresearcharray's fix onto the PR head (20d66e4, merge 661b3d1). src/cli.ts now gates |
|
Thanks @mvanhorn. Closing this as superseded, with a clear note on what happened:
That leaves this PR with nothing unique left, and it has since gone conflicting with main. So this is closed in favor of #47 (yours, merged) + #67. Thanks again for the coverage work. |
What
Adds CLI-level test coverage for the
mex logandmex timelineCommander wiring so the option-parsing risks called out in the PR #39 Copilot review have explicit regression tests. ExportsparseIntArg,parsePositiveIntArg, andprogramfromsrc/cli.ts, and gates the auto-parse on the module being the main entry point so the new tests can import without consuming the test runner's argv.Why
Closes #43. The PR #39 review flagged that
mex log/mex timelinehad module-level tests for the underlying runners but no coverage of the actual Commander wiring or option parsing - meaning a future tweak to--type, repeated--file, or--limit's positive-integer guard could silently break the CLI surface without any test signal. The new suite locks down those exact behaviors against the sameprograminstance the binary uses, so changes to the option declarations show up in CI.Type of change
The runtime behavior of
mex logandmex timelineis unchanged; the production bin still runsprogram.parse()on invocation (the new main-module guard evaluates true when run viadist/cli.js). The only export changes are the two parser helpers andprogramitself, which were previously module-private.How to test
npx vitest run test/cli.test.ts- 13 cases pass.npm test- full suite still green except the pre-existingtest/tui.test.tsfailure (missingreactin the test environment, unrelated to this change).npm run build- tsup build still producesdist/cli.jswith the shebang banner anddist/index.jslibrary entry; runningnode dist/cli.js timeline --limit abcstill errors at the option parser as before.Checklist
npm test)Source for the original review: #39 (comment)