Repository navigation
Fix test cleanup and clarify Node.js LTS setup text - #41
Merged
Merged
Conversation
Run Vitest with --globals so React Testing Library unmounts components after each test. Without it, two tests that render App in the same file fail with "Found multiple elements". tsconfig already loads the vitest/globals types. Tell learners to compare node -v with the LTS version on the Node.js download page, so the setup text stays correct without a pinned version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, have reported test and build validation, and only non-blocking feedback remains.
0 open findings
What changed in this PR
Fixes test isolation in the course’s book app and clarifies Node.js setup requirements.
Changes:
- Enables Vitest globals for automatic cleanup between tests.
- Clarifies LTS selection and major-version checks.
| File | Description |
|---|---|
samples/book-app-web/package.json |
Enables Vitest globals for test cleanup. |
00-setup/README.md |
Updates Node.js installation and version-check guidance. |
🧠 Review effort: Max
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Two fixes that came out of the review of #33.
1. Tests do not clean up between renders
tsconfig.jsonloads thevitest/globalstypes, but thetestscript did not run Vitest with--globals. Because of this, React Testing Library did not unmount components after each test.When two tests in the same file render
<App />, the second test fails:In Chapter 06, the agent writes Clear filters tests that render
Appmore than once, so learners can see this failure. #33 addedafterEach(cleanup)to work around it.Fix:
"test": "vitest --environment jsdom --globals"insamples/book-app-web/package.json.2. Node.js version text in setup
#40 changed the app to require Node 24 or later.
00-setup/README.mdsaid "Node.js LTS or later", but Node 22 is also an LTS version. Learners on Node 22 would getEBADENGINEwarnings.Fix: Tell learners to compare the first number of
node -vwith the LTS version on the Node.js download page. The text has no pinned version number, so it stays correct when a new LTS version is released.Validation
In
samples/book-app-web, on Node 24.11.1:Apptwice: 6/6 tests pass (before the fix, 1 test failed). I removed the temporary test after the check.npm test -- --run: 4/4 tests pass. The count is the same, so the Chapter 06 baseline screenshots are still correct.npm run build: passes