test: more tests for admin/events, also added alias for /src/__tests_ - $test#147
test: more tests for admin/events, also added alias for /src/__tests_ - $test#147
Conversation
📝 WalkthroughWalkthroughAdds a Vitest path alias Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 24 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/admin/events/__tests__/EventsTable.test.ts">
<violation number="1" location="frontend/src/components/admin/events/__tests__/EventsTable.test.ts:62">
P2: These assertions rely on the runtime default locale/time zone, which makes the test flaky across environments. Format the expected strings with an explicit locale and time zone (or mock Intl) to keep the test deterministic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts`:
- Around line 104-110: The test "shows '-' when correlation_id is null" sets
correlation_id to undefined, so update the test to match the case you want to
cover: either set detail.event.metadata.correlation_id = null to test the null
path, or rename the test to say "undefined" and/or add an additional test that
sets correlation_id = null to cover both; locate the code around
createMockEventDetail(), renderModal({ event: detail }) and the assertion using
screen.getAllByText('-') to make the change.
In `@frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts`:
- Around line 25-29: The test EventStatsCards.test ('displays total_events with
locale formatting and totalEvents denominator') relies on hardcoded
comma-formatted strings which are locale-dependent; update the assertions to
generate the expected strings using a fixed locale (e.g., call
toLocaleString('en-US') on the numbers used in renderCards) or use
locale-agnostic regex/normalization when querying text. Locate the test block
that calls renderCards(createMockStats({ total_events: 1500 }), 10000) and
replace the literal '1,500' and /of 10,000 total/ expectations with expressions
that derive the formatted text from the numeric values (or a regex that matches
the numeric digits regardless of separator) so the assertions no longer fail
under different CI locales.
🧹 Nitpick comments (6)
frontend/src/components/admin/events/__tests__/ReplayProgressBanner.test.ts (1)
139-154: Test name claims "red" styling but only asserts text presence.The test is named
'shows stderr in red when present'but only verifies that the text'NameError'is in the document. Consider adding a class or style assertion to actually validate the red styling, similar to the status badge tests above.frontend/src/components/admin/events/__tests__/ReplayPreviewModal.test.ts (1)
11-20: Consider importingReplayPreviewtype from the source instead of redefining it locally.This local interface definition could silently drift from the actual component's prop type. If the component exports or re-exports this type, importing it directly would catch type mismatches at compile time.
frontend/src/components/admin/events/__tests__/EventsTable.test.ts (1)
58-64: Locale-dependent timestamp assertions may be fragile in CI.
toLocaleDateString()andtoLocaleTimeString()produce different output depending on the environment's locale (e.g.,en-USvsen-GB). If CI runs with a different locale than your dev machine, this test will break. Consider either pinning the locale in the assertion or setting a fixed locale in the test environment.- const date = new Date('2024-06-15T14:30:00Z'); - expect(screen.getByText(date.toLocaleDateString())).toBeInTheDocument(); - expect(screen.getByText(date.toLocaleTimeString())).toBeInTheDocument(); + const date = new Date('2024-06-15T14:30:00Z'); + expect(screen.getByText(date.toLocaleDateString('en-US'))).toBeInTheDocument(); + expect(screen.getByText(date.toLocaleTimeString('en-US'))).toBeInTheDocument();This only works if the component also formats with the same locale. The key point is that both test and component should agree on the locale.
frontend/src/components/admin/events/__tests__/UserOverviewModal.test.ts (2)
109-114: Prefer a type-safe approach over theRecord<string, unknown>cast.The cast on line 111 bypasses TypeScript's type checking. If
createMockUserOverviewdoesn't support overrides, consider using a spread to keep it type-safe:- const overview = createMockUserOverview(); - (overview as Record<string, unknown>).rate_limit_summary = null; - renderModal({ overview }); + const overview = { ...createMockUserOverview(), rate_limit_summary: null }; + renderModal({ overview });This achieves the same result without silencing the compiler.
90-97: Exact count assertion ongetAllByText('100')is coupled to mock data.
expect(hundreds).toHaveLength(2)will break if the mock data or component layout changes to render "100" a different number of times. UsingtoBeGreaterThanOrEqual(1)(as done elsewhere in these tests) would be more resilient, though the stricter assertion does verify both values render. Just a trade-off to be aware of.frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts (1)
60-67: Consider usingcontainerfrom the render result instead of globaldocument.Line 64 queries
document.querySelector('pre')directly. While this works, using thecontainerreturned byrender()is more idiomatic with Testing Library and better isolates the test:- const pre = document.querySelector('pre'); + const { container } = renderModal({ event: detail }); + const pre = container.querySelector('pre');Though since
renderModalalready returns the render result, you'd just need to destructure it.
frontend/src/components/admin/events/__tests__/EventDetailsModal.test.ts
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts">
<violation number="1" location="frontend/src/components/admin/events/__tests__/EventStatsCards.test.ts:27">
P2: These assertions are locale-dependent because `toLocaleString()` uses the runtime default locale. Tests can fail on machines with different locale settings. Specify a fixed locale (or use Intl.NumberFormat) so the output is deterministic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
1 participant
Summary by cubic
Adds comprehensive tests for the admin/events UI (tables, modals, banners, filters, stats, user overview), covering empty states, callbacks, and formatting. Fixes and standardizes test imports by introducing a $test alias and updating existing tests.
Written for commit 84cfa37. Summary will update on new commits.
Summary by CodeRabbit
Tests
Chores