Repository navigation
test: remove agent traps from the test and e2e tooling - #524
Merged
Merged
Conversation
npm run test:unit ran Vitest only, which strips types, so a test that passes there could still fail typecheck in CI. It now runs the 1 s native typecheck first. npm run test keeps svelte-check, which already covers test files, and calls Vitest directly.
test:e2e reused a warm instance and its vault, so fixtures, layout, and runtime errors from manual verification or an interrupted run became the suite's baseline and failed later tests. It now stops this worktree's instance, removes the vault, provisions a fresh one, and stops the instance afterwards. A fresh vault adds no measurable time.
Tests in one file shared the file's sandbox, so two tests creating the same folder collided with "Folder already exists". The harness now creates a sandbox per test inside the file's sandbox, which teardown still removes. harness-isolation covers it with two tests that create the same folder.
Planting a defect by hand went wrong in the same few ways: an edit that matched more than once, a plant that failed typecheck so no test ran, a filter that matched no test read as a catch, and a git checkout restore that dropped uncommitted work. The script replaces exactly one match, type-checks, passes only when the named test is in the run's FAIL summary, restores from a backup copy, and rebuilds main.js after an e2e run.
Keeps two rules that tooling cannot enforce: a new test must fail on planted defects, proved with npm run plant and listed in the PR body, and an Obsidian executable is never run by hand because it opens the host's real profile. Trims history from the svelte-check, run --reload, Settings popout, and workflow concurrency notes.
The fresh-vault reset stopped the instance and deleted the vault even while another run used them. test:e2e now takes a run lock first and fails at once when another run holds it.
getContext().sandbox now throws outside a test instead of returning undefined or the previous test's folder. The isolation test also checks that both tests' folders share the file's sandbox as parent.
- Match the exact test name, so a sibling test's failure is no catch. - Run Vitest with NO_COLOR, so CI colors cannot hide the FAIL line. - Require the command to pass before the plant. - Restore on exit and on SIGINT, SIGTERM, and SIGHUP, from memory, keeping the file mode. - Accept several edits across files. - Fail when the post-e2e rebuild fails. tests/tooling/plant.test.ts runs the real script and catches each of these defects when planted back.
A second run rebuilt root main.js before it reached the lock, so the first run's later files loaded the second run's bundle. runTests now takes the lock, then builds, and a busy lock names the other run.
A failing pre-test check left the sandbox set for later hooks. Each sandbox test now checks its own parent, so it runs alone.
- Key files by resolved path, so two spellings of one file plant and restore it once. - Restore only a file that still holds the plant, otherwise keep the backups and fail. - Run the baseline after the restore. A build in the command then replaces anything built from the plant. - Launch npm through npm_execpath, so no shell splits arguments. - Print the FAIL lines that caught the plant.
The exit handler's second restore deleted the backups that a refused restore had just pointed at. A plant that was not caught skipped the baseline, so an e2e run left main.js built from the plant.
An interrupt during the build returned a null exit code, which exited 0, and a signal to the wrapper alone still reset the instance. Only a busy lock now reads as another run.
Deploying podnotes with
|
| Latest commit: |
f5aa585
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00b823b1.podnotes.pages.dev |
| Branch Preview URL: | https://dx-agent-traps.podnotes.pages.dev |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| process.on("exit", restore); | ||
| for (const signal of /** @type {NodeJS.Signals[]} */ (["SIGINT", "SIGTERM", "SIGHUP"])) { | ||
| process.on(signal, () => { | ||
| child?.kill(signal); |
There was a problem hiding this comment.
🟡 Medium scripts/plant.mjs:54
On interruption, plant restores the files and exits while an npm run test:e2e process tree can still be running, so its tests may continue against restored files and retain the run lock. The handler only signals the immediate child and does not wait for it to shut down; terminate the command’s process tree and await shutdown before restoring.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @scripts/plant.mjs around line 54:
On interruption, `plant` restores the files and exits while an `npm run test:e2e` process tree can still be running, so its tests may continue against restored files and retain the run lock. The handler only signals the immediate child and does not wait for it to shut down; terminate the command’s process tree and await shutdown before restoring.
Owner
Author
There was a problem hiding this comment.
Fixed in 9ad2ab6. On a signal, plant restores the files and then waits for its command to close, so test:e2e releases its run lock. A second signal exits at once. Repro: SIGTERM to plant during an e2e plant, then an immediate test:e2e. Before the fix it was refused with "Another npm run test:e2e is running". After it, the run passed.
There was a problem hiding this comment.
The lock race is addressed by waiting before exit, but the original ordering concern remains: the handler restores files before waiting for the command to close, so the e2e process can still run against restored sources. It also still signals only npm rather than explicitly terminating the command tree.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 476b5be. Configure here.
Owner
Author
|
PASS+NOTES - independent verification of 476b5be against master 2b67ade (Linux headless, Node 22.23.4, Obsidian 1.14.4 and 1.13.7) Claims
Notes (non-blocking)
|
plant exited at once on a signal, so a test:e2e child died on a closed pipe before it released its run lock, and the next run was refused. It now waits for the command to close, and a second signal exits at once. test:e2e also releases its lock when stopping the instance throws.
Owner
Author
|
PASS+NOTES - independent verification of 9ad2ab6 (Linux headless, Node 22.23.4, Obsidian 1.14.4)
Notes (non-blocking)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Agent instructions in AGENTS.md worked around traps in the test tooling. This change removes the traps in the tooling itself, so the rules can go.
npm run test:unitrunstypecheckbefore Vitest. A test with a type error no longer passes locally and fails in CI.npm run test:e2etakes a per-worktree run lock, builds, stops the isolated instance, provisions a fresh vault, runs the suite, and stops the instance. Leftovers from manual verification or an interrupted run no longer fail later tests. A second run in the same worktree fails at once instead of replacing the first run's bundle or vault.npm run plantproves that a test catches a defect. It applies one or more exact edits, type-checks, requires the named test to fail, restores the files, and requires the command to pass again. A tooling test runs the real script.Out of scope: tests that fake
appby hand still break when the plugin calls a new host API. Moving them to the shared fake intests/mocks/obsidian.tsis a larger follow-up.Verification
new TFile("a.md")passednpm run test:unit. After it,test:unitfails on TS2554.mode: "preview") before the change. After it, J-TR-06 passes with the leftover in place.npm run plantcaught two wrong sandbox fixes with them.test:e2eruns: the second exits 1 with the busy message and does not build, and the first passes.tests/tooling/plant.test.tscatches each defect that review found inplant, planted back withnpm run plantitself: colored output, a sibling test's failure, no signal handler, no baseline, a file named two ways, and deleted backups.npm run lint,format:check,typecheck,build,test(2,793),test:tooling(47), and the fullnpm run test:e2e(246 of 246, Obsidian 1.14.4, Linux headless) pass.Release impact
None. The changes touch only dev tooling, tests, and docs.
Note
Low Risk
Changes are limited to npm scripts, E2E harness/runner, tooling tests, and contributor docs; the shipped plugin bundle is untouched.
Overview
This PR hardens dev test tooling so local runs match CI expectations and E2E no longer depends on manual cleanup rules in AGENTS.md.
Unit tests:
npm run test:unitnow runstypecheckbefore Vitest, so type errors fail the unit suite instead of passing locally and failing only in CI.npm run testinlines Vitest without going through thetest:unitscript.E2E runner:
npm run test:e2ebuilds inside the runner, takes a per-worktree run lock (a second concurrent run fails fast), stops the isolated instance, deletes the vault, and starts fresh, then always stops the instance and releases the lock. Manual leftovers or interrupted runs no longer poison later suites.E2E harness: Each test gets its own sandbox folder under the file sandbox via
createSandboxApi; tests exposesandboxonly inside a test body. A new isolation test proves two tests in one file can create the same folder name.npm run plant: Newscripts/plant.mjsapplies exact one-match edits, type-checks, requires the named test to fail on the plant and the command to pass after restore, with backup/restore and signal handling.tests/tooling/plant.test.tsexercises the real script.Docs: AGENTS.md and CONTRIBUTING.md document plant workflow, the new E2E lifecycle, and trim obsolete workarounds (e.g. macOS pre-start steps, manual vault reset).
Reviewed by Cursor Bugbot for commit f5aa585. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add
plantdefect-verification CLI and per-test E2E sandboxesnpm run plant) that plants a deliberate edit into a file, runs a test command, and requires the exact named test to fail on the plant and pass on the restored baseline. It restores unchanged files, keeps backups when a file changed during the run, and handles SIGINT/SIGTERM/SIGHUP.SandboxApiunder the file sandbox via harness.ts; sandbox access outside a test is rejected. Adds an isolation test in harness-isolation.test.ts.test:unitto run typecheck before Vitest andtest:e2eto delegate build and lifecycle to the runner. Updates AGENTS.md and CONTRIBUTING.md to document the new workflows.npm run test:e2enow deletes and recreates the isolated vault every run and fails when another run holds the lock;test:unitfails earlier on type errors.Macroscope summarized f5aa585.