-
-
Couldn't load subscription status.
- Fork 1.3k
test(solid-router): fix timing in loaders test #5622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(solid-router): fix timing in loaders test #5622
Conversation
WalkthroughA previously skipped test "reproducer #4546" is enabled and enhanced with explicit synchronization points. Navigation completions and route data checks are awaited to ensure loader and data updates are reflected before assertions run. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5-10 minutes
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 3f63c79
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/solid-router/tests/loaders.test.tsx (1)
607-608: Consider usingwaitForfor more robust synchronization.The fixed 50ms timeout could be flaky on slower systems and doesn't verify that the state actually changed. Consider using
waitForto poll for the expected counter value instead:+import { cleanup, fireEvent, render, screen, waitFor } from '@solidjs/testing-library'Then replace the timeout:
- // Wait for router to invalidate and reload - await new Promise((resolve) => setTimeout(resolve, 50)) - const headerCounter = await screen.findByTestId('header-counter') - expect(headerCounter).toHaveTextContent('4') + // Wait for router to invalidate and reload + await waitFor(() => { + const headerCounter = screen.getByTestId('header-counter') + expect(headerCounter).toHaveTextContent('4') + })This polls until the condition is met rather than relying on an arbitrary delay.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/solid-router/tests/loaders.test.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/tests/loaders.test.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/loaders.test.tsx
⏰ 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: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/solid-router/tests/loaders.test.tsx (2)
405-405: Good fix - enabling the previously skipped test.Enabling this test with proper synchronization points improves test coverage and validates the fix for issue #4546.
577-578: LGTM - proper navigation synchronization.Awaiting route-specific text before assertions ensures navigation and rendering completes, making the test more reliable.
Also applies to: 592-593, 622-623
Summary by CodeRabbit