test(router-core): add some missing unit-tests to the new processRouteTree #5879
+122
−6
Conversation
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
Contributor
WalkthroughThis PR expands test coverage for router-core's route matching utilities by adding two new public exports ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 2d61d96
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/tests/new-process-route-tree.test.ts (1)
792-833: Consider restructuring withbeforeAllfor better test maintainability.While the sequential tests correctly validate the route masks processing workflow, the current structure has tests 2-5 depending on the side effects of test 1. This makes tests brittle if run in isolation or if new tests are inserted.
Consider this refactor:
describe('processRouteMasks', { sequential: true }, () => { const routeTree = { id: '__root__', isRoot: true, fullPath: '/', } as AnyRoute - const { processedTree } = processRouteTree(routeTree) + let processedTree: ReturnType<typeof processRouteTree>['processedTree'] + + beforeAll(() => { + const result = processRouteTree(routeTree) + processedTree = result.processedTree + + const routeMasks: Array<RouteMask<AnyRoute>> = [ + { from: '/a/b/c', routeTree }, + { from: '/a/b/d', routeTree }, + { from: '/a/$param/d', routeTree }, + { from: '/a/{-$optional}/d', routeTree }, + { from: '/a/b/{$}.txt', routeTree }, + ] + processRouteMasks(routeMasks, processedTree) + }) + it('processes a route masks list into a segment tree', () => { - const routeMasks: Array<RouteMask<AnyRoute>> = [ - { from: '/a/b/c', routeTree }, - { from: '/a/b/d', routeTree }, - { from: '/a/$param/d', routeTree }, - { from: '/a/{-$optional}/d', routeTree }, - { from: '/a/b/{$}.txt', routeTree }, - ] - processRouteMasks(routeMasks, processedTree) const aBranch = processedTree.masksTree?.staticInsensitive?.get('a') expect(aBranch).toBeDefined() expect(aBranch?.staticInsensitive?.get('b')).toBeDefined() expect(aBranch?.dynamic).toHaveLength(1) expect(aBranch?.optional).toHaveLength(1) })This separates setup from assertions and makes each test independently understandable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/tests/new-process-route-tree.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
🧬 Code graph analysis (1)
packages/router-core/tests/new-process-route-tree.test.ts (1)
packages/router-core/src/new-process-route-tree.ts (4)
findRouteMatch(606-623)processRouteTree(634-691)processRouteMasks(545-562)findFlatMatch(567-579)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/tests/new-process-route-tree.test.ts (3)
2-7: LGTM! New imports are appropriate.The new imports for
findFlatMatchandprocessRouteMasksare correctly added to support the expanded test coverage.
200-225: Excellent edge case coverage for optional param matching.These tests thoroughly verify the priority rules when optional params compete with dynamic params, ensuring that dynamic params are properly favored. The nested structure for testing optional params as standalone routes is clear and well-organized.
229-284: Comprehensive and well-structured trailing slash tests.The systematic coverage of index route behavior across static, dynamic, and optional routes is excellent. The parallel structure across the three describe blocks makes the test suite easy to understand and maintain.
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 by CodeRabbit