Enable coverage for API Keys page and improve deterministic test quality #2788
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved the Jest coverage exclusion for the settings directory and replaced a single API Keys page unit test with a comprehensive, centralized test suite and mocks; no runtime code or exported/public signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (2)
309-320: Consider extracting mutation mock helper to reduce nesting and duplication.SonarCloud flagged this for exceeding 4 levels of function nesting. The mutation mock logic duplicates the pattern in
setupMocks. Consider extracting a parameterized helper:+ const createMutationMock = (loading = false) => { + return jest.fn(async (vars) => { + const result = await mockCreateMutation(vars) + return result + }) + } + + const setupMutationMocks = (createLoading = false, revokeLoading = false) => { + mockUseMutation.mockImplementation((mutation, options) => { + if (mutation === CreateApiKeyDocument) { + const mutationFn = jest.fn(async (vars) => { + const result = await mockCreateMutation(vars) + if (options?.onCompleted) options.onCompleted(result.data) + return result + }) + return [mutationFn, { loading: createLoading }] + } + if (mutation === RevokeApiKeyDocument) { + const mutationFn = jest.fn(async (vars) => { + const result = await mockRevokeMutation(vars) + if (options?.onCompleted) options.onCompleted(result.data) + return result + }) + return [mutationFn, { loading: revokeLoading }] + } + return [jest.fn(), { loading: false }] + }) + }Then in the test:
test('disables create button when createLoading is true', async () => { - mockUseMutation.mockImplementation((mutation, options) => { - if (mutation === CreateApiKeyDocument) { - const mutationFn = jest.fn(async (vars) => { - const result = await mockCreateMutation(vars) - if (options?.onCompleted) options.onCompleted(result.data) - return result - }) - return [mutationFn, { loading: true }] - } - return [jest.fn(), { loading: false }] - }) + setupMutationMocks(true, false)
439-447: Test name doesn't match test behavior.This test is named "shows correct date formatting in table" but only verifies that header columns ('Created', 'Expires') exist. It doesn't validate any actual date formatting.
Either rename the test to match its actual behavior, or enhance it to verify formatted dates:
- test('shows correct date formatting in table', async () => { + test('displays table with Created and Expires columns', async () => { render(<ApiKeysPage />) await waitFor(() => { const table = screen.getByRole('table') expect(table).toBeInTheDocument() expect(screen.getByText('Created')).toBeInTheDocument() expect(screen.getByText('Expires')).toBeInTheDocument() }) })Or to actually test date formatting:
test('shows correct date formatting in table', async () => { render(<ApiKeysPage />) await waitFor(() => { const table = screen.getByRole('table') expect(table).toBeInTheDocument() - expect(screen.getByText('Created')).toBeInTheDocument() - expect(screen.getByText('Expires')).toBeInTheDocument() + // Verify actual formatted dates from mock data + expect(screen.getByText(/Jul 11, 2025/)).toBeInTheDocument() }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
[failure] 312-312: Refactor this code to not nest functions more than 4 levels deep.
🔇 Additional comments (10)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (10)
1-43: LGTM!The import structure and mock definitions are well-organized. The mocks appropriately stub external dependencies (Apollo, modal, toast, FontAwesome, and navigation) to isolate the component under test.
45-57: LGTM!Good use of fake timers with a fixed system time for deterministic date calculations. The clipboard mock and proper cleanup in
afterAllensure test isolation.
66-97: Well-structured test setup with centralized mocks.The
setupMocksfunction effectively centralizes Apollo mock configuration. The approach of wrapping mutation functions to invokeonCompletedcallbacks ensures realistic testing of async flows.
114-145: LGTM!Good coverage of all data states: loading, success with data, empty state, and error state. The tests are well-structured and focused on observable behavior.
147-195: LGTM!Comprehensive API key creation tests covering default expiry, custom expiry, and quick expiry presets. The date calculations correctly leverage the fixed system time from fake timers.
197-248: LGTM!Solid form validation tests covering boundary conditions (name length), character restrictions, and required field validation. Each test correctly verifies that the mutation is not called when validation fails.
250-277: LGTM!Good coverage of the revocation flow including both confirmation and cancellation paths. The use of
within()for scoped queries is a good practice.
279-297: LGTM!Good tests for key limit enforcement, verifying both the disabled button state and the warning message when the maximum is reached.
330-437: LGTM!Excellent edge case coverage including modal state reset, null expiration handling, boundary values, and character validation. These tests will catch regressions in nuanced behaviors.
449-456: LGTM!Simple and effective test for the empty name validation state.
Swyamk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (7)
4-4: Centralized toast and navigation mocks are clean and deterministicImporting
addToastdirectly and mocking@heroui/toastonce, plus the lightweightnext/navigationmock, keeps assertions straightforward and prevents per-test reconfiguration. The only minor limitation is that theuseRoutermock’spushisn’t externally captured; if you ever need to assert navigation calls, consider exporting thepushmock from this module-level mock so tests can inspect it.Also applies to: 33-35, 41-43
63-97: SharedsetupMocksis solid; consider deduplicatingmockUseMutationwiringThe
setupMockshelper cleanly centralizes theuseQuery/useMutationmocking and correctly passes throughonCompletedso page logic still runs, which is great. TheEdge Casestest that overridesmockUseMutationto forceloading: trueeffectively reimplements the same branching andonCompletedwiring. To keep this DRY, you could extract the mutation implementation into a small helper (e.g.,makeMockUseMutation({ createLoading: boolean })) and use it both insetupMocksand in that specific test.Also applies to: 310-320
114-145: Loading / data state coverage is thorough; minor styling‑coupling nitThese tests cover loading skeleton, populated data, empty state, and error state comprehensively. The only small nit is relying on
document.querySelectorAll('.animate-pulse')for the skeleton, which couples the test to a specific CSS class; if that class name is considered implementation detail, a role/test-id based selector would be slightly more robust. Otherwise this block looks good.
197-248: Validation tests correctly gate mutations and assert toast errorsThe three validation scenarios (name length, allowed characters, and required expiry) now all assert both the correct
addToastpayload and thatmockCreateMutationis not called, which aligns well with the PR goal of more robust validation tests. IfApiKeysPageis ever refactored to delegate validation to a dedicated form component, consider moving the very granular validation text assertions there and keeping page‑level tests focused on “mutation called vs not called” and toast presence, to reduce brittleness. Based on learnings about focusing validation tests at the form level, this is a future‑proofing consideration rather than a blocker.
250-277: Revocation confirm/cancel paths are exercised appropriatelyThe revocation tests cover both confirmation (asserting the correct
uuidis passed tomockRevokeMutation) and cancellation (ensuring it is not called). This gives good confidence around the destructive action flow. Optionally, you could also assert that the confirmation dialog disappears after either action, but that’s not strictly necessary for this PR.
279-297: Key limit and active count UI tests are solid; selector can be slightly tightenedVerifying both the disabled Create button and the “max keys reached” warning, plus the “2 / 3 active keys” text, gives nice coverage of quota-related UI. For the disabled button assertion, using
getByRole('button', { name: /Create New Key/i })instead ofgetByTextwould make it unambiguous that you’re targeting the button element (in case the text is reused in a non-button element later), but the current approach works.
308-456: Edge case coverage is comprehensive; minor DRY and Sonar concerns onlyThe
Edge Casesblock does a great job covering:
- loading‑disabled create button,
- modal cancel + state reset,
- no‑expiry keys,
- 100‑char boundary,
- dynamic button label with active count,
- allowed name characters,
- multi‑row table rendering & date headers, and
- disabled create button when name is empty.
Two small, non-blocking points:
- There is some duplication between the custom
mockUseMutationimplementation in the “createLoading true” test and the one defined insetupMocks; extracting a shared helper (as mentioned earlier) would keep these in sync.- SonarCloud’s complaint about nested functions (around this region) is typical for Jest tests due to
describe→test→waitForcallbacks. If that rule is noisy for test files, I’d lean toward tuning/ignoring it rather than contorting the tests; alternatively, you could move repeatedwaitForcallbacks into small top-level helpers to reduce nesting depth.Functionally, everything in this block looks correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
[failure] 312-312: Refactor this code to not nest functions more than 4 levels deep.
🔇 Additional comments (4)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (4)
45-53: Fake timers + fixed system time and clipboard mocking look correctUsing
jest.useFakeTimers()withjest.setSystemTimeinbeforeAlland restoring real timers inafterAllis a solid way to make the expiry-related tests deterministic. Thenavigator.clipboard.writeTextmock is also appropriately set up as a resolved Promise. No issues here from a correctness or maintainability standpoint.Also applies to: 55-57
99-113: Modal and form helpers significantly improve test readability
openCreateModalandfillKeyForm, combined with thebeforeEach/afterEachsetup, nicely reduce duplication and make each test’s intent clear. The use ofjest.clearAllMocks()is appropriate here to reset call history between tests while keeping the shared mock implementations.
147-195: API key creation flows are well covered and time‑stableThe tests for default 30‑day expiry, custom expiry, and the 90‑day/1‑year quick buttons are clear and correctly leverage the fixed system time plus
date-fns/addDaysto compute expected values. Building the expected ISO string explicitly avoids timezone surprises and matches how a typical date‑only input is normalized. No issues spotted.
299-306: Usage information test is straightforward and appropriateThe “API Key Usage” test simply ensures the guidance section renders with expected text and header name, which is a good low‑maintenance smoke test for that static content. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (5)
59-98: Mutation wrapper andsetupMockspattern are solid, but mock result shapes could better match generated typesThe
createMutationFnhelper andsetupMocksabstraction are a nice improvement: they emulate Apollo’sonCompletedbehavior and centralize the query/mutation wiring, which keeps individual tests focused on behavior rather than wiring.One improvement: the resolved values configured for
mockCreateMutation/mockRevokeMutation(viamockCreateApiKeyResultand the inline revoke result) don’t appear to include the full shape exposed by the generated documents (e.g.,ok,code,messageon bothCreateApiKeyandRevokeApiKey). Aligning these mocks with the actual GraphQL response types will reduce the risk of future tests masking logic that depends on those fields.Consider updating the mock data module / revoke mock to include the same fields and property names (
okinstead ofsuccess) used inapiKeyQueries.generated.ts.
115-146: Loading/data state coverage is thorough; be aware of CSS‑class brittlenessThe “Loading and Data States” block covers loading skeleton, populated list, empty state, and error state, which is a solid set of scenarios for this page.
The only minor concern is in the loading test where you assert on
.animate-pulseelements viadocument.querySelectorAll. This couples the test to a specific Tailwind/utility class and DOM access pattern. It’s fine for now, but if you start seeing frequent churn from styling tweaks, you may want to switch to a more semantic query (e.g., a test ID or skeleton text) to reduce brittleness.
198-249: Validation tests are strong but blur page‑ vs form‑level responsibilitiesThe validation tests (name length >100, illegal characters, and missing expiration date) are robust: they verify the correct toast message and, importantly, that
mockCreateMutationis not called on validation failure, which is valuable page‑level behavior.Given prior guidance in this repo, page tests ideally avoid re‑asserting low‑level validation details when a dedicated form component is mocked and has its own test suite. Here you’re also checking that the page blocks mutation calls and surfaces errors via toast, which is page‑specific behavior, so the tests are still justified. If there is a separate ApiKey form component with its own validations, consider moving the more granular message‑text assertions there and keeping these tests focused on “no mutation + some error toast was shown.”
Based on learnings, ...
251-278: Revocation flow assertions are good; align revoke mock payload with schemaThe revocation tests correctly cover both confirmation and cancel flows, asserting that the revoke mutation is called (or not) with the right
uuid. That’s the key behavior for this page.Given these tests don’t currently assert on success/failure toasts, it becomes more important that the revoke mock’s payload shape matches the generated
RevokeApiKeyschema (which usesok,code,message). Right now the configured resolved value insetupMocksuses{ success: true }, which doesn’t line up with the generated types. Updating this to includeok(and optionallycode/message) will prevent subtle discrepancies if you later add assertions based on the mutation result.
309-453: Edge‑case coverage is excellent; only minor opportunities to simplify or consolidateThe “Edge Cases” block covers a wide range of important scenarios (loading state disabling, modal cancel/reset, no‑expiry keys, 100‑char boundary, button text with counts, name character rules, multiple rows, table headers, and empty‑name disabled state). This is strong, high‑value coverage.
A few minor, optional observations:
- The
createLoadingMutationoverride duplicates some logic fromsetupMocks(specifically theCreateApiKeyDocumentbranch usingcreateMutationFn). It’s already quite small, but you could consider extracting a helper or composing on top ofsetupMocksif this pattern repeats elsewhere.- Some tests with very similar patterns (e.g., various “valid name” creation cases) could be parameterized with
it.eachto reduce repetition, though the current explicit tests are perfectly readable.- The “correct date formatting in table” test currently only asserts that the headers are present; if the intent is to assert actual formatting, you may later want to strengthen it to check at least one rendered date value.
These are all optional refinements; the behavior under test looks correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (2)
frontend/__tests__/unit/data/mockApiKeysData.ts (2)
mockApiKeys(1-19)mockCreateApiKeyResult(21-34)frontend/src/types/__generated__/apiKeyQueries.generated.ts (2)
CreateApiKeyDocument(26-26)RevokeApiKeyDocument(27-27)
🔇 Additional comments (4)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (4)
4-57: Scaffolding, mocks, timers, and clipboard setup look correct and deterministicThe centralized mocks for Apollo, toast, modal, FontAwesome, and Next router, plus the use of fake timers and fixed system time, are set up cleanly and make the tests deterministic. The clipboard mock resolving to
undefinedis appropriate forwriteText. UsingbeforeAll/afterAllfor timers andafterEach(jest.clearAllMocks)is a good balance between isolation and performance; no issues here.
100-114: Helper utilities for opening the modal and filling the form are a good DRY improvement
openCreateModalandfillKeyFormsignificantly reduce duplication and make the tests more readable. The helpers encapsulate the expected labels and button text clearly, and the explicit wait for the dialog role ensures the modal is ready before interacting. No changes needed here.
148-196: Expiry/creation tests are well structured and time‑deterministicThe API key creation tests (default 30‑day expiry, custom date, and quick‑expiry presets) are nicely done:
- Using
jest.setSystemTimeplusdate-fns/addDayskeeps expected dates deterministic.- Computing the expected ISO string from the formatted
yyyy-MM-ddmirrors what the component is likely doing with the<input type="date">value.- Quick‑expiry tests assert directly on the date input value, which is a clear way to validate preset behavior without depending on internal implementation details.
No functional issues here.
280-307: Key‑limit and usage information tests look good and improve regression protectionThe tests around maximum key limit (disabling the create button and showing a warning), displaying the active key count (
2 / 3 active keys), and the static “API Key Usage” information exercise important UI and copy conditions. They’re straightforward, rely on accessible queries, and should provide good regression coverage for future UX changes. No changes needed.
Swyamk
force-pushed
the
test/settings-coverage
branch
from
5e77cac to
e41f510
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (2)
300-307: Consider moving this test into a describe block for consistency.This test is placed outside of any
describeblock, while the rest of the tests are organized into logical groupings. Consider moving it into an existing block (e.g., "Loading and Data States" or "Key Limits and UI State") or creating a dedicated block like "UI Information Display" for consistency.
436-444: Test name doesn't match what's being tested.The test is named "shows correct date formatting in table" but only verifies that the table and column headers ("Created", "Expires") exist. It doesn't actually verify the date formatting of the values. Consider either:
- Renaming to something like "displays table with date columns"
- Adding assertions for actual date formatting (e.g., checking for formatted date strings like "Dec 4, 2025")
- test('shows correct date formatting in table', async () => { + test('displays table with date columns', async () => { render(<ApiKeysPage />) await waitFor(() => { const table = screen.getByRole('table') expect(table).toBeInTheDocument() expect(screen.getByText('Created')).toBeInTheDocument() expect(screen.getByText('Expires')).toBeInTheDocument() }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(2 hunks)frontend/jest.config.ts(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/jest.config.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (2)
frontend/__tests__/unit/data/mockApiKeysData.ts (1)
mockApiKeys(1-19)frontend/src/types/__generated__/apiKeyQueries.generated.ts (2)
CreateApiKeyDocument(26-26)RevokeApiKeyDocument(27-27)
🔇 Additional comments (9)
frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (9)
1-43: LGTM!The imports and mock setup are well-organized. The modal mock correctly handles the
isOpenconditional rendering, and the Apollo client mocks provide good flexibility for test configuration.
45-57: LGTM!Good use of fake timers with a frozen system time for deterministic date-based tests. The clipboard mock is correctly set up. Note that
jest.useFakeTimers()can sometimes cause issues withwaitForif timers need to be manually advanced, but the current setup should work since React Testing Library'swaitForhandles this gracefully in most cases.
66-75: Nice helper pattern for mutation callbacks.The
createMutationFnwrapper elegantly handles theonCompletedcallback invocation, enabling tests to verify both the mutation call and any side effects triggered by completion. This is a clean approach for testing Apollo mutations.
115-146: LGTM!Good coverage of the component's different states: loading, loaded with data, empty state, and error state. The assertions are clear and test the right behaviors.
148-196: LGTM!The API key creation tests are well-structured and deterministic thanks to the frozen system time. The date calculations using
addDaysandformatensure consistent expected values. Good coverage of default expiry, custom expiry, and quick expiry button functionality.
198-249: LGTM!Validation tests are thorough and correctly assert both the error toast display and that the mutation is not called when validation fails. This dual assertion pattern ensures the validation actually prevents the API call, which is exactly what was mentioned in the PR objectives about "strengthening the validation test."
251-278: LGTM!Good coverage of the revocation flow, testing both the confirmation and cancellation paths. The use of
within()for scoped queries is a nice touch for targeting the correct button within the table row.
280-298: LGTM!Good tests for the key limit functionality. The regex pattern on line 295 appropriately handles potential whitespace variations in the rendered text.
446-453: LGTM!Good edge case test verifying that the create button is disabled when the name input is empty, preventing accidental empty submissions.
1 participant
Summary
This PR enables Jest coverage for the
src/app/settings/**directory so that the existing API Keys page tests are now properly included in the frontend coverage reports. While the API Keys page already had good test coverage, those tests were being ignored due to a coverage exclusion in the Jest configuration.Along with fixing the coverage issue, the API Keys test suite itself has been improved to be more deterministic, easier to maintain, and more reliable overall. As a result, frontend coverage now accurately reflects the work that was already in place.
Fixed: #2781
Improvements Made
All suggested improvements have now been applied:
1. Cleaned up toast mocking
require('@heroui/toast')across multiple testsaddToastonce at the top of the file instead2. Removed a duplicate validation test