test: Add unit tests for multiple frontend components across openchoreo, openchoreo-ci and openchoreo-observerbility plugins#489
Merged
LakshanSS merged 15 commits intoopenchoreo:mainfrom Apr 3, 2026
Conversation
…p Card component - Eliminated auto deploy state and associated handlers from Environments component. - Updated EnvironmentsContext and SetupCard to remove auto deploy props and logic. - Refactored SetupCard to fetch auto deploy status directly and handle updates internally. - Cleaned up EnvironmentsList to remove unused auto deploy props. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
- Replace jest.mock('@backstage/core-plugin-api') with TestApiProvider from @backstage/test-utils for DI-based API mocking in tests
- Replace jest.mock('@backstage/plugin-catalog-react') with EntityProvider for entity context in tests
- Add shared frontend test utilities to @openchoreo/test-utils: createMockOpenChoreoClient(), mockComponentEntity(), mockSystemEntity()
Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
…rol and Git Secret pages - Introduced unit tests for AccessControlPage, MappingsTab, and RolesTab components to ensure proper rendering and functionality. - Implemented tests for various user permission scenarios, including loading states, role visibility, and action handling. - Added tests for EnvironmentActions and EnvironmentCard components to validate user interactions and permission checks. - Enhanced test coverage for Environments components, including EnvironmentCardContent and SetupCard. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
- Introduced unit tests for LogEntry, LogsActions, LogsFilter, LogsTable, ObservabilityProjectRuntimeLogsPage, and ObservabilityRuntimeLogsPage components to ensure proper rendering and functionality. - Implemented tests for various user interactions, including filtering, sorting, and log entry display. - Enhanced test coverage for log metadata display and error handling scenarios. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
- Introduced unit tests for AlertRow, AlertsActions, AlertsFilter, AlertsTable, and ObservabilityAlertsPage components to ensure proper rendering and functionality. - Added tests for IncidentRow, IncidentsActions, IncidentsFilter, IncidentsTable, and ObservabilityProjectIncidentsPage to validate user interactions and data display. - Enhanced test coverage for various scenarios, including alert and incident details, filtering, sorting, and loading states. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
- Introduced a comprehensive testing guide for frontend plugins, detailing recommended testing utilities and their usage. - Included sections on rendering components with `renderInTestApp`, `EntityProvider`, and `TestApiProvider`. - Provided guidelines on what to mock during testing and a quick reference table for component types and rendering methods. - Added a structured test example to illustrate best practices. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
stefinie123
requested review from
jhivandb,
kaviththiranga and
sameerajayasoma
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a testing guide and new frontend test utilities, introduces many Jest/RTL test suites across plugins, refactors tests to use Backstage providers (EntityProvider/TestApiProvider), adds entity fixtures and a mock OpenChoreo client, and moves auto‑deploy fetching/updating logic into SetupCard while removing auto‑deploy from Environments context and props. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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)
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 |
…s tests Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
…s, and RunMetadata components Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
plugins/openchoreo-observability/src/components/Metrics/utils.test.ts (1)
11-11: Remove redundant comment.The
// ---- Tests ----comment is unnecessary since the.test.tsfile extension already indicates this is a test file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Metrics/utils.test.ts` at line 11, Remove the redundant "// ---- Tests ----" comment in the test file; open plugins/openchoreo-observability/src/components/Metrics/utils.test.ts and delete that single-line comment (it provides no value because the .test.ts extension already indicates a test file), leaving the rest of the file and any test functions (e.g., any describe/it or test blocks in utils.test.ts) untouched.plugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsx (1)
106-117: Deduplicate overlapping loading-empty-state tests.The cases “shows loading skeletons when loading with no logs” and “does not show empty state when loading” assert the same outcome. Keeping one will simplify maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsx` around lines 106 - 117, Remove the duplicated test that asserts the same behavior for loading with no logs: keep a single test (either "shows loading skeletons when loading with no logs" or "does not show empty state when loading") that calls renderTable({ logs: [], loading: true }) and asserts expect(screen.queryByText('No logs found')).not.toBeInTheDocument();; delete the other redundant it(...) block to avoid overlap in LogsTable.test.tsx and rely on renderTable and screen.queryByText('No logs found') to verify the behavior.plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityRuntimeLogsPage.test.tsx (1)
68-79: Consider usingmockComponentEntityfrom@openchoreo/test-utilsfor consistency.Similar to the project-level page test, the inline entity could use the shared fixture:
import { mockComponentEntity } from '@openchoreo/test-utils'; const defaultEntity = mockComponentEntity({ name: 'api-service', annotations: { 'openchoreo.io/namespace': 'dev-ns', 'openchoreo.io/component': 'api-service', }, });This is optional since the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityRuntimeLogsPage.test.tsx` around lines 68 - 79, Replace the inline defaultEntity object in ObservabilityRuntimeLogsPage.test.tsx with the shared fixture mockComponentEntity from `@openchoreo/test-utils`: add an import for mockComponentEntity and construct defaultEntity via mockComponentEntity({ name: 'api-service', annotations: { 'openchoreo.io/namespace': 'dev-ns', 'openchoreo.io/component': 'api-service' } }); update any usages of the original defaultEntity accordingly (search for defaultEntity in the test file) so the test uses the shared fixture instead of the handcrafted object.plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsx (1)
71-81: Consider usingmockSystemEntityfrom@openchoreo/test-utilsfor consistency.The inline
defaultEntityworks correctly, but using the shared fixture would align with the new test utilities introduced in this PR:import { mockSystemEntity } from '@openchoreo/test-utils'; const defaultEntity = mockSystemEntity({ name: 'my-project', annotations: { 'openchoreo.io/namespace': 'dev-ns' }, });This is optional since the current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsx` around lines 71 - 81, Replace the inline test fixture defaultEntity with the shared mock from test utils: import mockSystemEntity from '@openchoreo/test-utils', then initialize defaultEntity by calling mockSystemEntity({ name: 'my-project', annotations: { 'openchoreo.io/namespace': 'dev-ns' } }) so tests use the common fixture; update the import list accordingly and keep the variable name defaultEntity used by the tests (function under test: ObservabilityProjectRuntimeLogsPage test setup).plugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsx (1)
90-108: Prefer behavior-first queries over MUI class selectors.Line 93, Line 101, and Line 107 assert via
.Mui-*classes, which is brittle across MUI changes. Prefer role/label/accessible-state assertions against specific controls.Refactor sketch
- const selects = document.querySelectorAll('.Mui-disabled'); - expect(selects.length).toBeGreaterThan(0); + expect(screen.getByPlaceholderText('Enter Trace ID to search')).toBeDisabled(); - expect(document.querySelector('.MuiSkeleton-root')).toBeInTheDocument(); + // Prefer querying a known loading placeholder/text/test-id exposed by the component. + // Example: expect(screen.getByTestId('components-loading-skeleton')).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsx` around lines 90 - 108, The tests currently rely on MUI implementation classes (e.g., '.Mui-disabled' and '.MuiSkeleton-root') which is brittle; update the three tests ("disables controls when disabled", "shows skeleton while components are loading", "shows skeleton while environments are loading") to use accessible queries from renderFilters instead of class selectors: for the disabled test, select the specific controls rendered by renderFilters (e.g., the select inputs via getByRole('combobox') or getByLabelText('<label text>')) and assert they are disabled with toBeDisabled(); for the skeleton tests, query for the Skeleton by role (e.g., getByRole('progressbar') or an aria-label/testid exposed by the Skeleton) and assert it is in the document using getByRole/getByLabelText rather than document.querySelector('.MuiSkeleton-root').plugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsx (1)
52-58: Consider usinggetByLabelTextorgetByRolefor more robust selector queries.The
getAllByText('Components').length >= 1pattern works but is less idiomatic. If the filter renders a label, usinggetByLabelText('Components')or querying by role would be more resilient to UI changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsx` around lines 52 - 58, The test "renders components selector when components exist" uses screen.getAllByText('Components').length which is brittle; update the assertion to use an accessible query such as screen.getByLabelText('Components') or screen.getByRole (e.g., getByRole('combobox', { name: 'Components' })) so the test targets the labeled control, and adjust the expect to assert presence (toBeInTheDocument()) — change this in the test that calls renderFilters() in IncidentsFilter.test.tsx.plugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsx (1)
71-77: Skeleton count assertion is tightly coupled to implementation.Asserting exactly 30 skeletons (5 rows × 6 cells) will break if the loading skeleton layout changes. Consider a looser assertion or documenting the expectation.
💡 Alternative: Assert skeletons exist without exact count
it('shows loading skeletons when loading', () => { renderTable({ loading: true, incidents: [] }); - // 5 skeleton rows × 6 cells each - const skeletons = document.querySelectorAll('.MuiSkeleton-root'); - expect(skeletons.length).toBe(30); + // Verify loading state renders skeletons + const skeletons = document.querySelectorAll('.MuiSkeleton-root'); + expect(skeletons.length).toBeGreaterThan(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsx` around lines 71 - 77, The test "shows loading skeletons when loading" currently asserts an exact count of 30 .MuiSkeleton-root nodes which is brittle; update the assertion in IncidentsTable.test.tsx (the test using renderTable and querying '.MuiSkeleton-root') to be looser—e.g., assert that at least one skeleton exists or that skeletons.length > 0 (or use a semantic query like getAllByRole/getAllByLabelText) instead of expecting exactly 30—so layout changes won’t break the test while still verifying loading state is rendered.plugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsx (1)
43-53: Consider strengthening the modified-change assertion.The test only verifies
/1 →/but doesn't assert the new value3appears. This could miss regressions where the new value isn't rendered correctly.💡 Suggestion to verify both old and new values
expect(screen.getByText(/replicas/)).toBeInTheDocument(); // The modified line contains "1 → 3" - expect(screen.getByText(/1 →/)).toBeInTheDocument(); + expect(screen.getByText(/1 → 3/)).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsx` around lines 43 - 53, The test in ChangesPreview.test.tsx currently only asserts the old value pattern (/1 →/) and may miss missing new values; update the test for the 'renders modified change with old → new values' case to assert the new value is present as well by checking for the full "1 → 3" rendering (or separate assertions that both the old value "1" and new value "3" appear) when rendering the ChangesPreview component with the Change { type: 'modified', path: 'replicas', oldValue: 1, newValue: 3 } so the expectation verifies both values show up.packages/test-utils/src/frontend/mockOpenChoreoClient.ts (2)
112-119: Consider stricter typing foroverridesparameter.The
overridesparameter acceptsRecord<string, jest.Mock>, allowing any key. A typo in a method name would silently be ignored. Consider using the same union type for better type safety:🔧 Proposed type improvement
export function createMockOpenChoreoClient( - overrides: Partial<Record<string, jest.Mock>> = {}, + overrides: Partial<Record<(typeof methodNames)[number], jest.Mock>> = {}, ): MockOpenChoreoClient {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts` around lines 112 - 119, The overrides parameter is too loose (Record<string, jest.Mock>) so typos in method names are ignored; change its type to a Partial keyed by the actual client methods (e.g., Partial<Record<keyof MockOpenChoreoClient, jest.Mock>> or Partial<{ [K in keyof MockOpenChoreoClient]: jest.Mock }>) and update the loop to iterate over methodNames while preserving proper typing so assignments like mock[name] = overrides[name] are type-checked against MockOpenChoreoClient and the compiler will catch misspelled method keys; keep the function signature createMockOpenChoreoClient, the methodNames usage, and return type MockOpenChoreoClient.
5-7: Consider adding automated verification for method list sync.The comment about keeping the list in sync is good documentation. The method list is currently complete and in sync with
OpenChoreoClientApi(all 76 methods match). For long-term maintainability and to prevent future drift, consider adding a test that imports theOpenChoreoClientApiinterface and validates that the mock's method list remains in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts` around lines 5 - 7, Add an automated test that imports the OpenChoreoClientApi type and the mock exported from mockOpenChoreoClient.ts and verifies the mock exposes the same method names as the interface; implement the test by extracting the interface's keys (e.g., via a helper type or by importing the real client implementation's prototype) and comparing as a set to Object.keys(mockOpenChoreoClient) to assert equality, failing the build if any method is missing or extra.plugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsx (1)
94-116: Prefer semantic field queries over index/DOM selectors.These cases are tightly coupled to the dialog’s DOM order via
getAllByRole('textbox')[n]anddocument.querySelector('input[type="password"]'). A small layout or MUI markup change will break the suite without any behavior regression. Query the inputs by label instead; the same pattern is repeated throughout this file.Example cleanup
- const inputs = screen.getAllByRole('textbox'); - await user.type(inputs[0], 'my-secret'); - const passwordInput = document.querySelector( - 'input[type="password"]', - ) as HTMLInputElement; - await user.type(passwordInput, 'my-token'); + await user.type(screen.getByLabelText(/secret name/i), 'my-secret'); + await user.type( + screen.getByLabelText(/password|access token/i), + 'my-token', + );Also applies to: 127-152, 165-166, 180-189, 218-245, 274-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsx` around lines 94 - 116, The tests in CreateSecretDialog.test.tsx rely on fragile DOM indexing and selectors (e.g., using getAllByRole('textbox')[0] and document.querySelector('input[type="password"]') in the "enables Create button when name and token are filled" test and several others) which couples them to markup order; replace those with semantic queries that target fields by label or accessible name (e.g., screen.getByLabelText('Secret Name'), screen.getByLabelText('Password') or screen.getByRole('textbox', { name: /Secret Name/i }) ), updating usages of inputs, passwordInput, and any other indexed lookups in renderDialog-related tests so each field is located by its label/accessibility name rather than index/selector; apply the same pattern to the other affected ranges mentioned.plugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsx (1)
149-159: Avoid assertingProgressinternals here.This case depends on a child component exposing
data-testid="progress", which is not part of the page contract and can change on a Backstage upgrade. MockProgressin this file like the other page suites do, or assert on a stable page-level signal instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsx` around lines 149 - 159, The test asserts a child component internals by querying data-testid="progress" from Progress; instead, modify the test to avoid relying on Progress implementation by either mocking Progress in this test file (similar to other page suites) or asserting a stable page-level signal (e.g., check for a loading message or a container element rendered by renderPage). Specifically, add a mock for the Progress component (or module) used by ObservabilityAlertsPage at the top of the test file and update the test to assert the mocked output (or assert a page-level text like "Loading…" or a loading container) instead of screen.getByTestId('progress'); keep mockUseAlertsPermission and renderPage usage the same.plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx (1)
40-42: Isolate mock client instances per test to prevent cross-test coupling
mockClientandmockAlertApiare shared singletons, while tests override method behavior (mockResolvedValue/mockRejectedValue). Becausejest.clearAllMocks()does not reset implementations, behavior can leak between tests as this suite grows.Proposed refactor
-const mockClient = createMockOpenChoreoClient(); -const mockAlertApi = { post: jest.fn() }; +let mockClient: ReturnType<typeof createMockOpenChoreoClient>; +let mockAlertApi: { post: jest.Mock }; describe('useDeleteEntityMenuItems', () => { beforeEach(() => { - jest.clearAllMocks(); + jest.clearAllMocks(); + mockClient = createMockOpenChoreoClient(); + mockAlertApi = { post: jest.fn() }; // Reset isMarkedForDeletion since clearAllMocks doesn't reset mockReturnValue const { isMarkedForDeletion } = require('../utils'); isMarkedForDeletion.mockReturnValue(false); });Also applies to: 113-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx` around lines 40 - 42, The test suite shares single-instance mocks (mockClient, mockAlertApi created via createMockOpenChoreoClient and jest.fn()) which allows overridden implementations to leak between tests; change to recreate these mocks per test (e.g., declare mockClient and mockAlertApi as variables and reassign new instances in a beforeEach or inside each test), or call jest.resetAllMocks() and recreate implementations before each test, ensuring any mockResolvedValue/mockRejectedValue calls only affect the current test and referencing the symbols mockClient, mockAlertApi, and createMockOpenChoreoClient to locate where to adjust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/openchoreo-observability/src/components/Alerts/AlertRow.test.tsx`:
- Around line 199-203: The test is clicking the last "View incident" button
which is brittle; instead scope the query to the expanded details container and
find the "View incident" button inside it (e.g., use
within(expandedDetailsContainer).getByRole('button', { name: /View incident/i })
or within(...).getByText('View incident')) and then call user.click on that
scoped button; locate the expanded container using the same selector the test
uses to expand rows (e.g., the element found earlier in the test or a
test-id/aria attribute for the expanded row) so you replace
viewIncidentButtons[...] with a scoped query and click the single matched
button.
In `@plugins/openchoreo-observability/src/components/Alerts/AlertsTable.test.tsx`:
- Around line 72-83: The second test ("shows loading skeletons when loading with
no alerts") is a duplicate that only repeats the empty-state assertion; update
it to actually assert skeleton rendering or remove it. Locate the two it blocks
in AlertsTable.test.tsx (the tests that call renderTable({ alerts: [], loading:
true })) and either delete the duplicate test, or change its assertions to check
for the loading skeleton elements produced by AlertsTable when loading (e.g.,
look for the component's skeleton/test-id, a "skeleton" CSS class, or an
accessible loading role/element) instead of asserting "No alerts found".
In
`@plugins/openchoreo-observability/src/components/Metrics/MetricsFilters.test.tsx`:
- Around line 67-73: The test in MetricsFilters.test.tsx uses a global CSS
selector '.Mui-disabled' which is brittle; update the test that calls
renderFilters({ disabled: true }) to instead query the specific controls (e.g.,
the environment select and the time range select) by their accessible labels
(using the same label text used in the component) and assert each returned
element is disabled (e.g., using .disabled or toBeDisabled). Locate the test
case 'disables controls when disabled' and replace the
document.querySelectorAll('.Mui-disabled') assertion with explicit queries for
the environment and time range controls and individual disabled assertions.
In `@plugins/openchoreo-observability/src/components/Traces/TracesTable.test.tsx`:
- Around line 114-124: The test sets
mockTraceSpans.getError.mockReturnValue('Network error') which persists across
tests (jest.clearAllMocks only clears calls, not return values); update the test
suite to avoid pollution by either using
mockTraceSpans.getError.mockReturnValueOnce('Network error') inside the 'shows
span error when expanded with error' test, or reset the mock return value in a
shared hook (e.g., add mockTraceSpans.getError.mockReset() or
jest.resetAllMocks()/jest.restoreAllMocks() in a beforeEach/afterEach) so
subsequent tests don't see the lingering return value.
In
`@plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx`:
- Around line 222-232: After awaiting the deletion call, make the subsequent
side-effect assertions wait-aware to avoid flakiness: change the direct
assertions on mockAlertApi.post and mockNavigate so they run inside a waitFor
(or use await waitFor(() => expect(...))). Specifically, wrap the expectations
for mockAlertApi.post (message 'Component "my-service" has been marked for
deletion' and severity 'success') and mockNavigate('/catalog') in waitFor so
they only assert after effects triggered by useDeleteEntityMenuItems complete;
reference mockClient.deleteComponent, mockAlertApi.post, mockNavigate and
waitFor in your fix.
In
`@plugins/openchoreo/src/components/Environments/components/EnvironmentCard.test.tsx`:
- Around line 7-13: The first jest.mock of '@openchoreo/backstage-design-system'
is redundant because it's overwritten by the later mock that also defines
StatusBadge; remove the earlier mock block (the one that only returns Card with
data-testid="ds-card") so only the later mock (which provides both Card and
StatusBadge) remains; ensure the remaining mock still exports Card and
StatusBadge as expected for EnvironmentCard.test.tsx.
In `@plugins/openchoreo/src/components/Environments/components/SetupCard.tsx`:
- Around line 40-58: The toggle is being set using checked={autoDeploy ?? false}
which makes it interactive before the current entity's value is loaded; add a
per-entity "loaded" flag (e.g., autoDeployLoaded or isAutoDeployLoaded) and only
enable the switch once the fetchComponentData resolves successfully for that
specific entity; in fetchComponentData record the current entity (or use an
incrementing requestId/AbortController) so that stale responses are ignored and
only setAutoDeploy and setAutoDeployLoaded when the response matches the
outstanding request; apply the same guard/loaded flag and stale-response check
to the other fetch/update logic referenced around the 115-120 area so the
control stays disabled until the initial read for the current entity completes.
In `@plugins/openchoreo/src/components/GitSecrets/GitSecretsPage.test.tsx`:
- Around line 129-134: The test "renders namespace selector with sorted
namespaces" only checks that the Namespace selector renders (via renderContent
and screen.getByLabelText('Namespace')) but doesn't verify ordering; update the
test in GitSecretsPage.test.tsx to either assert the actual option order (e.g.,
use screen.getByLabelText('Namespace') to locate the select, then
getAllByRole('option') or querySelectorAll('option') and compare their
textContent to the expected sorted array) or change the test name to reflect
that it only checks rendering (e.g., "renders namespace selector"); modify the
assertions accordingly in the same test block that calls renderContent().
In `@plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx`:
- Around line 75-106: The tests claim the list is refreshed after mutations but
never assert it; update the tests for useGitSecrets to explicitly verify
mockClient.listGitSecrets is called after createSecret and deleteSecret by
checking call counts or last call (e.g., after invoking
result.current.createSecret(...) and awaiting, assert mockClient.listGitSecrets
was called again), and do the same in the deleteSecret test—locate the test
blocks that call result.current.createSecret and result.current.deleteSecret and
add assertions verifying mockClient.listGitSecrets was invoked to confirm the
refetch behavior.
---
Nitpick comments:
In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts`:
- Around line 112-119: The overrides parameter is too loose (Record<string,
jest.Mock>) so typos in method names are ignored; change its type to a Partial
keyed by the actual client methods (e.g., Partial<Record<keyof
MockOpenChoreoClient, jest.Mock>> or Partial<{ [K in keyof
MockOpenChoreoClient]: jest.Mock }>) and update the loop to iterate over
methodNames while preserving proper typing so assignments like mock[name] =
overrides[name] are type-checked against MockOpenChoreoClient and the compiler
will catch misspelled method keys; keep the function signature
createMockOpenChoreoClient, the methodNames usage, and return type
MockOpenChoreoClient.
- Around line 5-7: Add an automated test that imports the OpenChoreoClientApi
type and the mock exported from mockOpenChoreoClient.ts and verifies the mock
exposes the same method names as the interface; implement the test by extracting
the interface's keys (e.g., via a helper type or by importing the real client
implementation's prototype) and comparing as a set to
Object.keys(mockOpenChoreoClient) to assert equality, failing the build if any
method is missing or extra.
In
`@plugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsx`:
- Around line 43-53: The test in ChangesPreview.test.tsx currently only asserts
the old value pattern (/1 →/) and may miss missing new values; update the test
for the 'renders modified change with old → new values' case to assert the new
value is present as well by checking for the full "1 → 3" rendering (or separate
assertions that both the old value "1" and new value "3" appear) when rendering
the ChangesPreview component with the Change { type: 'modified', path:
'replicas', oldValue: 1, newValue: 3 } so the expectation verifies both values
show up.
In
`@plugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsx`:
- Around line 149-159: The test asserts a child component internals by querying
data-testid="progress" from Progress; instead, modify the test to avoid relying
on Progress implementation by either mocking Progress in this test file (similar
to other page suites) or asserting a stable page-level signal (e.g., check for a
loading message or a container element rendered by renderPage). Specifically,
add a mock for the Progress component (or module) used by
ObservabilityAlertsPage at the top of the test file and update the test to
assert the mocked output (or assert a page-level text like "Loading…" or a
loading container) instead of screen.getByTestId('progress'); keep
mockUseAlertsPermission and renderPage usage the same.
In
`@plugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsx`:
- Around line 52-58: The test "renders components selector when components
exist" uses screen.getAllByText('Components').length which is brittle; update
the assertion to use an accessible query such as
screen.getByLabelText('Components') or screen.getByRole (e.g.,
getByRole('combobox', { name: 'Components' })) so the test targets the labeled
control, and adjust the expect to assert presence (toBeInTheDocument()) — change
this in the test that calls renderFilters() in IncidentsFilter.test.tsx.
In
`@plugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsx`:
- Around line 71-77: The test "shows loading skeletons when loading" currently
asserts an exact count of 30 .MuiSkeleton-root nodes which is brittle; update
the assertion in IncidentsTable.test.tsx (the test using renderTable and
querying '.MuiSkeleton-root') to be looser—e.g., assert that at least one
skeleton exists or that skeletons.length > 0 (or use a semantic query like
getAllByRole/getAllByLabelText) instead of expecting exactly 30—so layout
changes won’t break the test while still verifying loading state is rendered.
In `@plugins/openchoreo-observability/src/components/Metrics/utils.test.ts`:
- Line 11: Remove the redundant "// ---- Tests ----" comment in the test file;
open plugins/openchoreo-observability/src/components/Metrics/utils.test.ts and
delete that single-line comment (it provides no value because the .test.ts
extension already indicates a test file), leaving the rest of the file and any
test functions (e.g., any describe/it or test blocks in utils.test.ts)
untouched.
In
`@plugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsx`:
- Around line 106-117: Remove the duplicated test that asserts the same behavior
for loading with no logs: keep a single test (either "shows loading skeletons
when loading with no logs" or "does not show empty state when loading") that
calls renderTable({ logs: [], loading: true }) and asserts
expect(screen.queryByText('No logs found')).not.toBeInTheDocument();; delete the
other redundant it(...) block to avoid overlap in LogsTable.test.tsx and rely on
renderTable and screen.queryByText('No logs found') to verify the behavior.
In
`@plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsx`:
- Around line 71-81: Replace the inline test fixture defaultEntity with the
shared mock from test utils: import mockSystemEntity from
'@openchoreo/test-utils', then initialize defaultEntity by calling
mockSystemEntity({ name: 'my-project', annotations: { 'openchoreo.io/namespace':
'dev-ns' } }) so tests use the common fixture; update the import list
accordingly and keep the variable name defaultEntity used by the tests (function
under test: ObservabilityProjectRuntimeLogsPage test setup).
In
`@plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityRuntimeLogsPage.test.tsx`:
- Around line 68-79: Replace the inline defaultEntity object in
ObservabilityRuntimeLogsPage.test.tsx with the shared fixture
mockComponentEntity from `@openchoreo/test-utils`: add an import for
mockComponentEntity and construct defaultEntity via mockComponentEntity({ name:
'api-service', annotations: { 'openchoreo.io/namespace': 'dev-ns',
'openchoreo.io/component': 'api-service' } }); update any usages of the original
defaultEntity accordingly (search for defaultEntity in the test file) so the
test uses the shared fixture instead of the handcrafted object.
In
`@plugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsx`:
- Around line 90-108: The tests currently rely on MUI implementation classes
(e.g., '.Mui-disabled' and '.MuiSkeleton-root') which is brittle; update the
three tests ("disables controls when disabled", "shows skeleton while components
are loading", "shows skeleton while environments are loading") to use accessible
queries from renderFilters instead of class selectors: for the disabled test,
select the specific controls rendered by renderFilters (e.g., the select inputs
via getByRole('combobox') or getByLabelText('<label text>')) and assert they are
disabled with toBeDisabled(); for the skeleton tests, query for the Skeleton by
role (e.g., getByRole('progressbar') or an aria-label/testid exposed by the
Skeleton) and assert it is in the document using getByRole/getByLabelText rather
than document.querySelector('.MuiSkeleton-root').
In
`@plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx`:
- Around line 40-42: The test suite shares single-instance mocks (mockClient,
mockAlertApi created via createMockOpenChoreoClient and jest.fn()) which allows
overridden implementations to leak between tests; change to recreate these mocks
per test (e.g., declare mockClient and mockAlertApi as variables and reassign
new instances in a beforeEach or inside each test), or call jest.resetAllMocks()
and recreate implementations before each test, ensuring any
mockResolvedValue/mockRejectedValue calls only affect the current test and
referencing the symbols mockClient, mockAlertApi, and createMockOpenChoreoClient
to locate where to adjust.
In `@plugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsx`:
- Around line 94-116: The tests in CreateSecretDialog.test.tsx rely on fragile
DOM indexing and selectors (e.g., using getAllByRole('textbox')[0] and
document.querySelector('input[type="password"]') in the "enables Create button
when name and token are filled" test and several others) which couples them to
markup order; replace those with semantic queries that target fields by label or
accessible name (e.g., screen.getByLabelText('Secret Name'),
screen.getByLabelText('Password') or screen.getByRole('textbox', { name: /Secret
Name/i }) ), updating usages of inputs, passwordInput, and any other indexed
lookups in renderDialog-related tests so each field is located by its
label/accessibility name rather than index/selector; apply the same pattern to
the other affected ranges mentioned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4ed90fa-f8db-4037-a56f-aace7fa58da4
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (70)
TESTING_GUIDE.mdpackages/test-utils/package.jsonpackages/test-utils/src/frontend/entityFixtures.tspackages/test-utils/src/frontend/index.tspackages/test-utils/src/frontend/mockOpenChoreoClient.tspackages/test-utils/src/index.tsplugins/openchoreo-ci/package.jsonplugins/openchoreo-ci/src/components/BuildStatusChip/BuildStatusChip.test.tsxplugins/openchoreo-ci/src/components/BuildWithCommitDialog/BuildWithCommitDialog.test.tsxplugins/openchoreo-ci/src/components/OverviewTab/OverviewTab.test.tsxplugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsxplugins/openchoreo-ci/src/components/RunsTab/RunsTab.test.tsxplugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsxplugins/openchoreo-ci/src/components/WorkflowDetailsRenderer/WorkflowDetailsRenderer.test.tsxplugins/openchoreo-ci/src/components/WorkflowRunDetailsPage/WorkflowRunDetailsPage.test.tsxplugins/openchoreo-ci/src/components/Workflows/Workflows.test.tsxplugins/openchoreo-ci/src/utils/schemaExtensions.test.tsplugins/openchoreo-observability/src/components/Alerts/AlertRow.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsActions.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsFilter.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsTable.test.tsxplugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsActions.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsxplugins/openchoreo-observability/src/components/Incidents/ObservabilityProjectIncidentsPage.test.tsxplugins/openchoreo-observability/src/components/Metrics/MetricsActions.test.tsxplugins/openchoreo-observability/src/components/Metrics/MetricsFilters.test.tsxplugins/openchoreo-observability/src/components/Metrics/ObservabilityMetricsPage.test.tsxplugins/openchoreo-observability/src/components/Metrics/utils.test.tsplugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsxplugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsxplugins/openchoreo-observability/src/components/RCA/RCAPage.test.tsxplugins/openchoreo-observability/src/components/RCA/RCATable.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsActions.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsFilter.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityRuntimeLogsPage.test.tsxplugins/openchoreo-observability/src/components/Traces/ObservabilityTracesPage.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesActions.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesTable.test.tsxplugins/openchoreo-observability/src/components/Traces/utils.test.tsplugins/openchoreo/package.jsonplugins/openchoreo/src/components/AccessControl/AccessControlPage.test.tsxplugins/openchoreo/src/components/AccessControl/MappingsTab/MappingsTab.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/ClusterRolesContent.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RolesTab.test.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsxplugins/openchoreo/src/components/Environments/Environments.test.tsxplugins/openchoreo/src/components/Environments/Environments.tsxplugins/openchoreo/src/components/Environments/EnvironmentsContext.tsxplugins/openchoreo/src/components/Environments/EnvironmentsList.test.tsxplugins/openchoreo/src/components/Environments/EnvironmentsList.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentActions.test.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentCard.test.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentCardContent.test.tsxplugins/openchoreo/src/components/Environments/components/SetupCard.test.tsxplugins/openchoreo/src/components/Environments/components/SetupCard.tsxplugins/openchoreo/src/components/Environments/types.tsplugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsxplugins/openchoreo/src/components/GitSecrets/GitSecretsPage.test.tsxplugins/openchoreo/src/components/GitSecrets/SecretsTable.test.tsxplugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsxplugins/openchoreo/src/components/Projects/OverviewCards/DeploymentPipelineCard.test.tsxplugins/openchoreo/src/components/Projects/ProjectComponentsCard/ProjectComponentsCard.test.tsxplugins/openchoreo/src/components/Workflows/OverviewCard/WorkflowsOverviewCard.test.tsx
💤 Files with no reviewable changes (3)
- plugins/openchoreo/src/components/Environments/EnvironmentsList.tsx
- plugins/openchoreo/src/components/Environments/types.ts
- plugins/openchoreo/src/components/Environments/EnvironmentsContext.tsx
plugins/openchoreo-observability/src/components/Alerts/AlertRow.test.tsx
Outdated
Show resolved
Hide resolved
plugins/openchoreo-observability/src/components/Alerts/AlertsTable.test.tsx
Show resolved
Hide resolved
plugins/openchoreo-observability/src/components/Metrics/MetricsFilters.test.tsx
Show resolved
Hide resolved
plugins/openchoreo-observability/src/components/Traces/TracesTable.test.tsx
Show resolved
Hide resolved
plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx
Show resolved
Hide resolved
plugins/openchoreo/src/components/Environments/components/EnvironmentCard.test.tsx
Outdated
Show resolved
Hide resolved
plugins/openchoreo/src/components/GitSecrets/GitSecretsPage.test.tsx
Outdated
Show resolved
Hide resolved
plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/openchoreo/src/components/Environments/EnvironmentsList.test.tsx (1)
353-389: Minor: Test numbering is inconsistent (skips#11).The test comments jump from
// 10.to// 12., skipping// 11.. This may indicate a removed test or a numbering oversight. Consider renumbering for consistency, or adding a comment explaining the gap if test#11was intentionally removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo/src/components/Environments/EnvironmentsList.test.tsx` around lines 353 - 389, The test comments are misnumbered (jumping from "// 10." to "// 12."); update the inline comment before the onRefresh test to "// 11." (or renumber the surrounding test comments consistently) so the sequence around the tests for onOpenOverrides and onRefresh in EnvironmentsList.test.tsx reads consecutively; locate the tests using symbols like onOpenOverrides, onRefresh, capturedEnvironmentCardProps, and mockHandleRefreshEnvironment to find and edit the comment.plugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsx (2)
7-10: Align mocked hook state with the realuseWorkflowRunshape.All mocked returns omit
refetch, so tests are less strict about the hook contract. Consider using a shared state factory that always includesrefetchand only overrides per test case.♻️ Suggested patch
const mockUseWorkflowRun = jest.fn(); +const makeWorkflowRunState = (overrides: Record<string, unknown> = {}) => ({ + workflowRun: null, + loading: false, + error: null, + refetch: jest.fn(), + ...overrides, +}); // ... - mockUseWorkflowRun.mockReturnValue({ - workflowRun: null, - loading: true, - error: null, - }); + mockUseWorkflowRun.mockReturnValue( + makeWorkflowRunState({ loading: true }), + );Also applies to: 53-58, 66-70, 80-84, 96-103, 113-121, 131-138, 150-157, 167-177, 185-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsx` around lines 7 - 10, The mock for the useWorkflowRun hook (mockUseWorkflowRun) returns objects that omit the refetch method; update the tests to provide a shared mock state factory (e.g., createMockWorkflowRunState) that always includes a refetch noop and any other required properties, then have mockUseWorkflowRun(name) return createMockWorkflowRunState(overrides) so each test can override fields but refetch is always present; update calls in RunMetadataContent.test.tsx (all occurrences of mockUseWorkflowRun returns mentioned in the review) to use this factory to match the real useWorkflowRun shape.
18-20: Add one assertion path for extracted Git metadata.
extractGitFieldValuesis always mocked to{}, so the Git-field rendering path is never validated in this suite.♻️ Suggested patch
-jest.mock('../../utils/schemaExtensions', () => ({ - extractGitFieldValues: (_params: any, _mapping: any) => ({}), -})); +const mockExtractGitFieldValues = jest.fn(); +jest.mock('../../utils/schemaExtensions', () => ({ + extractGitFieldValues: (...args: any[]) => mockExtractGitFieldValues(...args), +})); beforeEach(() => { jest.clearAllMocks(); + mockExtractGitFieldValues.mockReturnValue({}); }); +it('renders git fields when extracted values are available', () => { + mockUseWorkflowRun.mockReturnValue({ + workflowRun: { ...baseBuild }, + loading: false, + error: null, + }); + mockExtractGitFieldValues.mockReturnValue({ + repoUrl: 'https://github.com/openchoreo/backstage-plugins', + branch: 'main', + }); + + renderContent(); + + expect(screen.getByText('https://github.com/openchoreo/backstage-plugins')).toBeInTheDocument(); + expect(screen.getByText('main')).toBeInTheDocument(); +});Also applies to: 95-110, 184-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsx` around lines 18 - 20, The suite currently always mocks extractGitFieldValues to return {} so the Git-field rendering path in RunMetadataContent is never exercised; update the tests in RunMetadataContent.test.tsx to add a case where extractGitFieldValues returns a non-empty mapping (e.g., a sample git metadata object) and assert that the component renders the expected Git fields. Concretely, change the mock behavior for extractGitFieldValues (use jest.mock/spy or mockReturnValueOnce on the mocked module) within the specific test that should validate Git metadata, mount/render RunMetadataContent, and add expectations for the rendered Git keys/values to verify the Git-field rendering path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@plugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsx`:
- Around line 7-10: The mock for the useWorkflowRun hook (mockUseWorkflowRun)
returns objects that omit the refetch method; update the tests to provide a
shared mock state factory (e.g., createMockWorkflowRunState) that always
includes a refetch noop and any other required properties, then have
mockUseWorkflowRun(name) return createMockWorkflowRunState(overrides) so each
test can override fields but refetch is always present; update calls in
RunMetadataContent.test.tsx (all occurrences of mockUseWorkflowRun returns
mentioned in the review) to use this factory to match the real useWorkflowRun
shape.
- Around line 18-20: The suite currently always mocks extractGitFieldValues to
return {} so the Git-field rendering path in RunMetadataContent is never
exercised; update the tests in RunMetadataContent.test.tsx to add a case where
extractGitFieldValues returns a non-empty mapping (e.g., a sample git metadata
object) and assert that the component renders the expected Git fields.
Concretely, change the mock behavior for extractGitFieldValues (use
jest.mock/spy or mockReturnValueOnce on the mocked module) within the specific
test that should validate Git metadata, mount/render RunMetadataContent, and add
expectations for the rendered Git keys/values to verify the Git-field rendering
path.
In `@plugins/openchoreo/src/components/Environments/EnvironmentsList.test.tsx`:
- Around line 353-389: The test comments are misnumbered (jumping from "// 10."
to "// 12."); update the inline comment before the onRefresh test to "// 11."
(or renumber the surrounding test comments consistently) so the sequence around
the tests for onOpenOverrides and onRefresh in EnvironmentsList.test.tsx reads
consecutively; locate the tests using symbols like onOpenOverrides, onRefresh,
capturedEnvironmentCardProps, and mockHandleRefreshEnvironment to find and edit
the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbea2ab3-4b48-4654-85f3-2c175455761d
📒 Files selected for processing (8)
plugins/openchoreo-ci/src/components/RunMetadataContent/RunMetadataContent.test.tsxplugins/openchoreo-ci/src/components/RunsTab/RunsTab.test.tsxplugins/openchoreo-ci/src/components/WorkflowRunDetailsPage/WorkflowRunDetailsPage.test.tsxplugins/openchoreo-observability/src/components/Metrics/MetricsActions.test.tsxplugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesActions.test.tsxplugins/openchoreo/src/components/Environments/EnvironmentsList.test.tsxplugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx
✅ Files skipped from review due to trivial changes (3)
- plugins/openchoreo-observability/src/components/Metrics/MetricsActions.test.tsx
- plugins/openchoreo-observability/src/components/Traces/TracesActions.test.tsx
- plugins/openchoreo-ci/src/components/RunsTab/RunsTab.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsx
- plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx
- plugins/openchoreo-ci/src/components/WorkflowRunDetailsPage/WorkflowRunDetailsPage.test.tsx
… components Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
…s, and Alerts components - Improved assertions in useDeleteEntityMenuItems tests to ensure proper alert and navigation behavior. - Removed unnecessary mocks in EnvironmentCard tests for cleaner code. - Simplified namespace selector test in GitSecretsPage for clarity. - Verified list refresh behavior in useGitSecrets tests after create and delete actions. - Updated AlertRow tests to improve button interaction verification. - Enhanced AlertsTable tests to check for loading skeletons instead of empty state. - Refined MetricsFilters tests to ensure proper control disabling checks. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
plugins/openchoreo/src/components/Environments/components/SetupCard.tsx (1)
46-60:⚠️ Potential issue | 🟠 MajorClear stale
autoDeploystate when a new read starts.Line 48 only resets
autoDeployLoaded. If the previous entity hadautoDeploy=trueand the nextgetComponentDetails()call rejects or returns an object withoutautoDeploy, the old value survives and becomes interactive again once Line 60 flipsautoDeployLoadedback totrue. ResetautoDeployat fetch start and in the fallback path so each entity starts from a neutral state.Possible fix
useEffect(() => { let cancelled = false; + setAutoDeploy(undefined); setAutoDeployLoaded(false); const fetchComponentData = async () => { try { const componentData = await client.getComponentDetails(entity); - if (!cancelled && componentData && 'autoDeploy' in componentData) { - setAutoDeploy((componentData as any).autoDeploy); + if (!cancelled) { + setAutoDeploy( + componentData && 'autoDeploy' in componentData + ? (componentData as any).autoDeploy + : undefined, + ); } } catch { - // Silently fail - autoDeploy will remain undefined + if (!cancelled) { + setAutoDeploy(undefined); + } } finally { if (!cancelled) { setAutoDeployLoaded(true); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo/src/components/Environments/components/SetupCard.tsx` around lines 46 - 60, The effect's fetchComponentData may leave a stale autoDeploy value from a previous entity; in the useEffect before calling client.getComponentDetails (inside fetchComponentData) call setAutoDeploy(undefined) to clear previous state, and also in the catch/fallback path ensure setAutoDeploy(undefined) is invoked when componentData is absent or when an error occurs; update the logic around fetchComponentData, setAutoDeployLoaded, and the client.getComponentDetails handling so each new entity starts with autoDeploy cleared.
🧹 Nitpick comments (8)
plugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsx (1)
50-54: Strengthen disabled-path behavior test with a click assertion.You already assert disabled state; consider also asserting
onRefreshis not called on attempted click to guard regressions.✅ Suggested test hardening
it('disables Refresh button when disabled', async () => { - render(<RCAActions totalCount={0} disabled onRefresh={jest.fn()} />); - - expect(screen.getByRole('button', { name: /refresh/i })).toBeDisabled(); + const user = userEvent.setup(); + const onRefresh = jest.fn(); + render(<RCAActions totalCount={0} disabled onRefresh={onRefresh} />); + + const refreshButton = screen.getByRole('button', { name: /refresh/i }); + expect(refreshButton).toBeDisabled(); + await user.click(refreshButton); + expect(onRefresh).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsx` around lines 50 - 54, Update the test in RCAActions.test.tsx for the "disables Refresh button when disabled" case to also verify the click handler is not invoked: keep rendering <RCAActions totalCount={0} disabled onRefresh={onRefreshMock} /> (use a jest.fn() named e.g. onRefreshMock), simulate a user click on the Refresh button (via userEvent.click or fireEvent.click) and assert onRefreshMock was not called; reference the RCAActions component and the test case name so the change is applied to the existing disabled-path test.plugins/openchoreo-ci/src/utils/schemaExtensions.test.ts (1)
75-79: Consider testing all unsafe path segments for consistency.The
getNestedValuetests cover all three unsafe keys (__proto__,constructor,prototype), butsetNestedValueonly tests__proto__. While the underlyingvalidatePathPartsfunction handles all three, adding explicit tests would improve consistency and documentation of the security boundary.♻️ Optional: Add tests for other unsafe segments
it('throws on unsafe path segment', () => { expect(() => setNestedValue({}, '__proto__.polluted', true)).toThrow( 'Unsafe path segment', ); }); + + it('throws on constructor path segment', () => { + expect(() => setNestedValue({}, 'a.constructor.b', true)).toThrow( + 'Unsafe path segment', + ); + }); + + it('throws on prototype path segment', () => { + expect(() => setNestedValue({}, 'prototype.polluted', true)).toThrow( + 'Unsafe path segment', + ); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-ci/src/utils/schemaExtensions.test.ts` around lines 75 - 79, Add explicit tests to verify setNestedValue rejects all unsafe path segments (not just "__proto__"); specifically add test cases calling setNestedValue({}, 'constructor.polluted', true) and setNestedValue({}, 'prototype.polluted', true) and assert they throw the same 'Unsafe path segment' error. Reference setNestedValue and the underlying validatePathParts to locate where to add these tests so behavior matches the existing getNestedValue coverage.plugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsx (2)
77-82: Consider strengthening the disabled state assertion.The current test confirms something is disabled but doesn't verify which controls or how many. Consider checking specific controls like the search input, or verifying the expected count of disabled elements.
💡 Example: verify specific control is disabled
it('disables controls when disabled', () => { renderFilters({ disabled: true }); - const selects = document.querySelectorAll('.Mui-disabled'); - expect(selects.length).toBeGreaterThan(0); + // Verify search input is disabled + expect(screen.getByPlaceholderText('Search RCA reports...')).toBeDisabled(); + + // Verify MUI selects are disabled + const disabledControls = document.querySelectorAll('.Mui-disabled'); + expect(disabledControls.length).toBeGreaterThanOrEqual(3); // Environment, Status, Time Range });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsx` around lines 77 - 82, The test currently only asserts that some element has the Mui-disabled class; update it to assert specific controls are disabled after calling renderFilters({ disabled: true }) by selecting them explicitly (e.g., query the search input by role "textbox" or placeholder and assert input.disabled or aria-disabled, query filter dropdowns by role "combobox" and assert they are disabled, and optionally assert the exact count of disabled controls). Target identifiers: the test helper renderFilters, the search input element (role/placeholder), and any dropdowns/buttons used in RCAFilters so the assertions verify the exact controls and expected disabled count.
59-75: Consider more specific queries for selector labels.The
getAllByText(...).length >= 1pattern is weak—it passes if the text appears anywhere in the document. For MUI Select components, consider usinggetByRoleorgetByLabelTextfor more precise targeting.💡 Example using getByRole for the combobox
it('renders environment selector', () => { renderFilters(); - expect(screen.getAllByText('Environment').length).toBeGreaterThanOrEqual(1); + expect(screen.getByRole('combobox', { name: /environment/i })).toBeInTheDocument(); });If MUI's Select doesn't expose the accessible name reliably, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsx` around lines 59 - 75, The tests in RCAFilters.test.tsx use getAllByText(...).length checks which are too broad; update the three assertions in the 'renders environment selector', 'renders status selector', and 'renders time range selector' tests to use more specific queries (e.g., getByRole('combobox', { name: /Environment/i }) or getByLabelText('Environment') for the Environment selector, and analogous queries for 'Status' and 'Time Range'), replacing the length checks with direct existence assertions (e.g., expect(...).toBeInTheDocument()) and fall back to the existing getAllByText approach only if MUI Select does not expose a reliable accessible name; keep the tests using renderFilters() and the same test names.plugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsx (1)
101-105: Consider using a more precise assertion.
getAllByText(...).length).toBeGreaterThanOrEqual(1)works but is less precise thangetByRoleorgetByText. If there's only one "View RCA" button expected, a direct assertion is clearer.💡 Optional: More precise assertion
it('shows View RCA button when incidentTriggerAiRca is true', () => { renderRow(); - expect(screen.getAllByText('View RCA').length).toBeGreaterThanOrEqual(1); + expect(screen.getByRole('button', { name: /View RCA/i })).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsx` around lines 101 - 105, The test "shows View RCA button when incidentTriggerAiRca is true" uses expect(screen.getAllByText('View RCA').length).toBeGreaterThanOrEqual(1) which is imprecise; update the assertion to assert the single expected element directly (e.g., use screen.getByText('View RCA') or screen.getByRole(...) for a button) after calling renderRow(), so the test verifies the exact presence of the "View RCA" button (reference the test name and renderRow() and the 'View RCA' text to locate the code to change).plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx (1)
28-29: Prefer condition-based waiting over no-opactfor async effects.Using
await act(async () => {});(e.g., Line 28) is brittle and can hide timing issues.waitForwith a concrete assertion is more stable for hook-side async state transitions.Suggested pattern update
-import { renderHook, act } from '@testing-library/react'; +import { renderHook, act, waitFor } from '@testing-library/react'; it('fetches secrets on mount', async () => { renderHook(() => useGitSecrets('test-ns')); - await act(async () => {}); - - expect(mockClient.listGitSecrets).toHaveBeenCalledWith('test-ns'); + await waitFor(() => { + expect(mockClient.listGitSecrets).toHaveBeenCalledWith('test-ns'); + }); });Also applies to: 42-43, 62-63, 84-95, 121-125, 143-144, 149-150, 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx` around lines 28 - 29, Replace the no-op `await act(async () => {});` calls in useGitSecrets.test.tsx with condition-based waiting using `waitFor` (from `@testing-library/react`) and concrete assertions on the hook's state or mocked calls; specifically update usages where `act` is used to flush async effects for the `useGitSecrets` tests (multiple occurrences) to await `waitFor(() => expect(...).to... )` that checks the expected state change or that a mock function was called, ensuring each test asserts a concrete condition rather than relying on an empty act to resolve async behavior.plugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsx (2)
158-176: Add one test forComponentNamecolumn fallback when metadata is missing.You verify fallback in expanded metadata, but not in the row column path (
LogEntryField.ComponentName) whenlog.metadatais undefined. Adding that case would close this regression gap.Proposed test addition
+ it('falls back to componentName prop in ComponentName column when metadata is missing', () => { + renderLogEntry({ + log: { ...sampleLog, metadata: undefined }, + componentName: 'fallback-component', + selectedFields: [...allFields, LogEntryField.ComponentName], + }); + + expect(screen.getByText('fallback-component')).toBeInTheDocument(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsx` around lines 158 - 176, Add a test in LogEntry.test.tsx that verifies the row-level ComponentName column falls back to the prop when log.metadata is undefined: render using renderLogEntry with log: { ...sampleLog, metadata: undefined } and props environmentName, projectName, componentName, then assert that screen.getByText for LogEntryField.ComponentName's visible cell shows the provided componentName (e.g., 'fallback-component') without expanding the row; reference renderLogEntry, sampleLog and LogEntryField.ComponentName to locate the column renderer to test.
119-125: Use a stable query for the clickable row instead of index-based text targeting.The test uses
getAllByText('Server started on port 8080')[0]twice to interact with the collapsible log entry. If the component renders the same text in multiple places (e.g., log message cell and detail view), the index will become unreliable. Use a role-based query scoped to the row (e.g.,within(screen.getByRole('row')).getByText(...)) or add adata-testidto the clickableTableRowelement for a stable, intent-clear selector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsx` around lines 119 - 125, The test in LogEntry.test.tsx is using index-based selection via getAllByText('Server started on port 8080')[0], which is brittle; update the test to target the clickable row deterministically by either querying the row role and scoping the text lookup (e.g., use screen.getByRole('row', {name: ...}) or within(screen.getByRole('row', ...)).getByText(...)) or add a stable data-testid to the clickable TableRow and use getByTestId to perform the clicks and assertions; update both the expand and collapse interactions to use the new stable selector so they no longer rely on array indexing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@plugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsx`:
- Around line 117-136: The mocks for mockUseComponentAlerts in
ObservabilityAlertsPage.test.tsx are missing the totalCount property from the
hook contract; update every mockReturnValue call for mockUseComponentAlerts to
include totalCount (e.g., totalCount: 2 for the two-alert mock, and appropriate
counts for other variants) alongside alerts, loading, error, fetchAlerts, and
refresh so tests validate the count wiring (refer to mockUseComponentAlerts and
the test cases in ObservabilityAlertsPage.test.tsx).
In
`@plugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsx`:
- Around line 51-57: The test in IncidentRow.test.tsx uses new
Date(...).toLocaleString() which is locale/timezone dependent and causes flaky
failures; update the assertion in the 'renders incident timestamp' test (where
renderRow() is called and screen.getByText(...) is used) to compare a stable,
deterministic string or a tolerant match: either format the date with
Intl.DateTimeFormat with an explicit locale and timeZone (e.g., 'en-US' or
'en-GB' and 'UTC') to produce a fixed expected string, or replace getByText(...)
with a regex/partial text match that only asserts the stable parts of the
timestamp (e.g., year-month-day or hour:minute) to avoid exact locale-specific
formatting.
In
`@plugins/openchoreo-observability/src/components/Incidents/ObservabilityProjectIncidentsPage.test.tsx`:
- Around line 118-120: The test mock for useUpdateIncident is incomplete: update
the mockUseUpdateIncident.mockReturnValue to return the full shape used by the
component (include updateIncident, updating and error) so tests reflect the hook
behavior; for example return { updateIncident: jest.fn(), updating: false,
error: null } (or appropriate initial values) so components and table
interactions that read updating/error behave correctly.
- Around line 110-116: The mocked return values for mockUseProjectIncidents are
missing the totalCount property required by the hook contract; update every
mockReturnValue call for mockUseProjectIncidents (including the instances around
the given diff and at the other occurrences referenced) to include totalCount: 0
(or an appropriate number per test) alongside incidents, loading, error,
fetchIncidents, and refresh so the test shape matches useProjectIncidents and
count-related UI logic is exercised correctly.
In
`@plugins/openchoreo/src/components/Environments/components/SetupCard.test.tsx`:
- Around line 111-172: Tests are asserting/clicking the Auto Deploy switch
before the mocked getComponentDetails has settled, causing false positives;
update each affected test (e.g., those using renderSetupCard,
mockClient.getComponentDetails) to wait for the post-load state by awaiting the
resolved UI instead of just asserting not.toBeChecked — for example wait for an
element that only appears after load (or use await screen.findByRole('checkbox',
{ name: /auto deploy/i }) or waitFor(() =>
expect(mockClient.getComponentDetails).toHaveBeenCalled())) so the switch is
enabled and reflects the fetched value before clicking or asserting success
(apply same change to the other test block referenced at lines ~231-244).
In `@plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx`:
- Around line 154-162: Add a positive forbidden-path test: update
useGitSecrets.test.tsx to add a new it block that mocks
mockClient.listGitSecrets to reject with an error object that simulates a 403
(e.g., an Error with response.status = 403 or a custom error type your hook
checks), render the hook via renderHook(() => useGitSecrets('test-ns')), await
the effect with act, and assert result.current.isForbidden === true; reference
the existing test structure and mocks (mockClient.listGitSecrets and
useGitSecrets) to mirror setup and teardown so the new test covers the forbidden
branch.
In
`@plugins/openchoreo/src/components/Workflows/OverviewCard/WorkflowsOverviewCard.test.tsx`:
- Around line 164-169: The test uses a non-existent latestBuild.commit field;
update the fixture in WorkflowsOverviewCard.test.tsx to match the real
useWorkflowsSummary ModelsBuild shape (remove the fake commit property or rename
it to the actual field name used by ModelsBuild, e.g., commitSha/revision) and
update the assertion that checks truncation to target that real field (or remove
the truncation assertion if that data is never provided by the hook). Ensure
changes reference the latestBuild fixture and the assertion around the
truncation behavior tied to useWorkflowsSummary.
---
Duplicate comments:
In `@plugins/openchoreo/src/components/Environments/components/SetupCard.tsx`:
- Around line 46-60: The effect's fetchComponentData may leave a stale
autoDeploy value from a previous entity; in the useEffect before calling
client.getComponentDetails (inside fetchComponentData) call
setAutoDeploy(undefined) to clear previous state, and also in the catch/fallback
path ensure setAutoDeploy(undefined) is invoked when componentData is absent or
when an error occurs; update the logic around fetchComponentData,
setAutoDeployLoaded, and the client.getComponentDetails handling so each new
entity starts with autoDeploy cleared.
---
Nitpick comments:
In `@plugins/openchoreo-ci/src/utils/schemaExtensions.test.ts`:
- Around line 75-79: Add explicit tests to verify setNestedValue rejects all
unsafe path segments (not just "__proto__"); specifically add test cases calling
setNestedValue({}, 'constructor.polluted', true) and setNestedValue({},
'prototype.polluted', true) and assert they throw the same 'Unsafe path segment'
error. Reference setNestedValue and the underlying validatePathParts to locate
where to add these tests so behavior matches the existing getNestedValue
coverage.
In
`@plugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsx`:
- Around line 101-105: The test "shows View RCA button when incidentTriggerAiRca
is true" uses expect(screen.getAllByText('View
RCA').length).toBeGreaterThanOrEqual(1) which is imprecise; update the assertion
to assert the single expected element directly (e.g., use screen.getByText('View
RCA') or screen.getByRole(...) for a button) after calling renderRow(), so the
test verifies the exact presence of the "View RCA" button (reference the test
name and renderRow() and the 'View RCA' text to locate the code to change).
In `@plugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsx`:
- Around line 50-54: Update the test in RCAActions.test.tsx for the "disables
Refresh button when disabled" case to also verify the click handler is not
invoked: keep rendering <RCAActions totalCount={0} disabled
onRefresh={onRefreshMock} /> (use a jest.fn() named e.g. onRefreshMock),
simulate a user click on the Refresh button (via userEvent.click or
fireEvent.click) and assert onRefreshMock was not called; reference the
RCAActions component and the test case name so the change is applied to the
existing disabled-path test.
In `@plugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsx`:
- Around line 77-82: The test currently only asserts that some element has the
Mui-disabled class; update it to assert specific controls are disabled after
calling renderFilters({ disabled: true }) by selecting them explicitly (e.g.,
query the search input by role "textbox" or placeholder and assert
input.disabled or aria-disabled, query filter dropdowns by role "combobox" and
assert they are disabled, and optionally assert the exact count of disabled
controls). Target identifiers: the test helper renderFilters, the search input
element (role/placeholder), and any dropdowns/buttons used in RCAFilters so the
assertions verify the exact controls and expected disabled count.
- Around line 59-75: The tests in RCAFilters.test.tsx use
getAllByText(...).length checks which are too broad; update the three assertions
in the 'renders environment selector', 'renders status selector', and 'renders
time range selector' tests to use more specific queries (e.g.,
getByRole('combobox', { name: /Environment/i }) or getByLabelText('Environment')
for the Environment selector, and analogous queries for 'Status' and 'Time
Range'), replacing the length checks with direct existence assertions (e.g.,
expect(...).toBeInTheDocument()) and fall back to the existing getAllByText
approach only if MUI Select does not expose a reliable accessible name; keep the
tests using renderFilters() and the same test names.
In
`@plugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsx`:
- Around line 158-176: Add a test in LogEntry.test.tsx that verifies the
row-level ComponentName column falls back to the prop when log.metadata is
undefined: render using renderLogEntry with log: { ...sampleLog, metadata:
undefined } and props environmentName, projectName, componentName, then assert
that screen.getByText for LogEntryField.ComponentName's visible cell shows the
provided componentName (e.g., 'fallback-component') without expanding the row;
reference renderLogEntry, sampleLog and LogEntryField.ComponentName to locate
the column renderer to test.
- Around line 119-125: The test in LogEntry.test.tsx is using index-based
selection via getAllByText('Server started on port 8080')[0], which is brittle;
update the test to target the clickable row deterministically by either querying
the row role and scoping the text lookup (e.g., use screen.getByRole('row',
{name: ...}) or within(screen.getByRole('row', ...)).getByText(...)) or add a
stable data-testid to the clickable TableRow and use getByTestId to perform the
clicks and assertions; update both the expand and collapse interactions to use
the new stable selector so they no longer rely on array indexing.
In `@plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx`:
- Around line 28-29: Replace the no-op `await act(async () => {});` calls in
useGitSecrets.test.tsx with condition-based waiting using `waitFor` (from
`@testing-library/react`) and concrete assertions on the hook's state or mocked
calls; specifically update usages where `act` is used to flush async effects for
the `useGitSecrets` tests (multiple occurrences) to await `waitFor(() =>
expect(...).to... )` that checks the expected state change or that a mock
function was called, ensuring each test asserts a concrete condition rather than
relying on an empty act to resolve async behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2170664-133c-4968-ab1a-4ed000f8b5a6
📒 Files selected for processing (49)
TESTING_GUIDE.mdplugins/openchoreo-ci/src/components/BuildWithCommitDialog/BuildWithCommitDialog.test.tsxplugins/openchoreo-ci/src/components/OverviewTab/OverviewTab.test.tsxplugins/openchoreo-ci/src/components/RunsTab/RunsTab.test.tsxplugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsxplugins/openchoreo-ci/src/components/WorkflowDetailsRenderer/WorkflowDetailsRenderer.test.tsxplugins/openchoreo-ci/src/utils/schemaExtensions.test.tsplugins/openchoreo-observability/src/components/Alerts/AlertRow.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsActions.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsFilter.test.tsxplugins/openchoreo-observability/src/components/Alerts/AlertsTable.test.tsxplugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsActions.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsxplugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsxplugins/openchoreo-observability/src/components/Incidents/ObservabilityProjectIncidentsPage.test.tsxplugins/openchoreo-observability/src/components/Metrics/MetricsActions.test.tsxplugins/openchoreo-observability/src/components/Metrics/MetricsFilters.test.tsxplugins/openchoreo-observability/src/components/Metrics/ObservabilityMetricsPage.test.tsxplugins/openchoreo-observability/src/components/Metrics/utils.test.tsplugins/openchoreo-observability/src/components/RCA/RCAActions.test.tsxplugins/openchoreo-observability/src/components/RCA/RCAFilters.test.tsxplugins/openchoreo-observability/src/components/RCA/RCAPage.test.tsxplugins/openchoreo-observability/src/components/RCA/RCATable.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogEntry.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsActions.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsFilter.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsxplugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsxplugins/openchoreo-observability/src/components/Traces/ObservabilityTracesPage.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesActions.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsxplugins/openchoreo-observability/src/components/Traces/TracesTable.test.tsxplugins/openchoreo-observability/src/components/Traces/utils.test.tsplugins/openchoreo/src/components/AccessControl/MappingsTab/MappingsTab.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/ClusterRolesContent.test.tsxplugins/openchoreo/src/components/AccessControl/RolesTab/RolesTab.test.tsxplugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentActions.test.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentCard.test.tsxplugins/openchoreo/src/components/Environments/components/EnvironmentCardContent.test.tsxplugins/openchoreo/src/components/Environments/components/SetupCard.test.tsxplugins/openchoreo/src/components/Environments/components/SetupCard.tsxplugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsxplugins/openchoreo/src/components/GitSecrets/GitSecretsPage.test.tsxplugins/openchoreo/src/components/GitSecrets/SecretsTable.test.tsxplugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsxplugins/openchoreo/src/components/Workflows/OverviewCard/WorkflowsOverviewCard.test.tsx
✅ Files skipped from review due to trivial changes (19)
- plugins/openchoreo-observability/src/components/Traces/TracesActions.test.tsx
- plugins/openchoreo-observability/src/components/Alerts/AlertsFilter.test.tsx
- plugins/openchoreo-observability/src/components/Alerts/AlertsActions.test.tsx
- plugins/openchoreo-observability/src/components/Traces/TracesFilters.test.tsx
- plugins/openchoreo-observability/src/components/RuntimeLogs/LogsFilter.test.tsx
- plugins/openchoreo-observability/src/components/Incidents/IncidentsFilter.test.tsx
- TESTING_GUIDE.md
- plugins/openchoreo-observability/src/components/Traces/utils.test.ts
- plugins/openchoreo-observability/src/components/Traces/TracesTable.test.tsx
- plugins/openchoreo-ci/src/components/BuildWithCommitDialog/BuildWithCommitDialog.test.tsx
- plugins/openchoreo-observability/src/components/Alerts/AlertsTable.test.tsx
- plugins/openchoreo/src/components/AccessControl/MappingsTab/MappingsTab.test.tsx
- plugins/openchoreo-ci/src/components/WorkflowConfigPage/EditWorkflowConfigs/ChangesPreview.test.tsx
- plugins/openchoreo/src/components/DeleteEntity/hooks/useDeleteEntityMenuItems.test.tsx
- plugins/openchoreo-observability/src/components/Metrics/utils.test.ts
- plugins/openchoreo/src/components/Environments/components/EnvironmentCardContent.test.tsx
- plugins/openchoreo/src/components/Environments/components/EnvironmentActions.test.tsx
- plugins/openchoreo-observability/src/components/Traces/ObservabilityTracesPage.test.tsx
- plugins/openchoreo/src/components/GitSecrets/SecretsTable.test.tsx
🚧 Files skipped from review as they are similar to previous changes (17)
- plugins/openchoreo-observability/src/components/Metrics/MetricsFilters.test.tsx
- plugins/openchoreo/src/components/AccessControl/RolesTab/RolesTab.test.tsx
- plugins/openchoreo-observability/src/components/Incidents/IncidentsActions.test.tsx
- plugins/openchoreo-observability/src/components/RCA/RCATable.test.tsx
- plugins/openchoreo-observability/src/components/RCA/RCAPage.test.tsx
- plugins/openchoreo-ci/src/components/OverviewTab/OverviewTab.test.tsx
- plugins/openchoreo/src/components/GitSecrets/CreateSecretDialog.test.tsx
- plugins/openchoreo/src/components/GitSecrets/GitSecretsPage.test.tsx
- plugins/openchoreo-observability/src/components/RuntimeLogs/LogsTable.test.tsx
- plugins/openchoreo-observability/src/components/Incidents/IncidentsTable.test.tsx
- plugins/openchoreo-observability/src/components/Metrics/MetricsActions.test.tsx
- plugins/openchoreo-observability/src/components/Alerts/AlertRow.test.tsx
- plugins/openchoreo-ci/src/components/WorkflowDetailsRenderer/WorkflowDetailsRenderer.test.tsx
- plugins/openchoreo-observability/src/components/RuntimeLogs/LogsActions.test.tsx
- plugins/openchoreo/src/components/AccessControl/RolesTab/ClusterRolesContent.test.tsx
- plugins/openchoreo-observability/src/components/RuntimeLogs/ObservabilityProjectRuntimeLogsPage.test.tsx
- plugins/openchoreo/src/components/Environments/components/EnvironmentCard.test.tsx
plugins/openchoreo-observability/src/components/Alerts/ObservabilityAlertsPage.test.tsx
Show resolved
Hide resolved
plugins/openchoreo-observability/src/components/Incidents/IncidentRow.test.tsx
Show resolved
Hide resolved
...openchoreo-observability/src/components/Incidents/ObservabilityProjectIncidentsPage.test.tsx
Show resolved
Hide resolved
...openchoreo-observability/src/components/Incidents/ObservabilityProjectIncidentsPage.test.tsx
Show resolved
Hide resolved
plugins/openchoreo/src/components/Environments/components/SetupCard.test.tsx
Show resolved
Hide resolved
plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx
Show resolved
Hide resolved
plugins/openchoreo/src/components/Workflows/OverviewCard/WorkflowsOverviewCard.test.tsx
Show resolved
Hide resolved
- Introduced a new test case to verify that the isForbidden state is set to true when a 403 error is encountered. - Enhanced the existing test suite for useGitSecrets to improve error handling coverage. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
LakshanSS
reviewed
Apr 3, 2026
plugins/openchoreo/src/components/Environments/components/SetupCard.tsx
Outdated
Show resolved
Hide resolved
LakshanSS
reviewed
Apr 3, 2026
Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
- Replaced static method names array with a dynamic Proxy-based approach for creating a fully-mocked OpenChoreoClientApi. - This change ensures that any method accessed on the mock is automatically a jest.fn(), keeping the mock up-to-date with the interface without manual updates. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/test-utils/src/frontend/mockOpenChoreoClient.ts (2)
24-24: Consider constraining the type toOpenChoreoClientApifor compile-time safety.The current
Record<string, jest.Mock>type allows tests to access any property name, including typos likemockClient.getEnvironmentss, without TypeScript errors. The Proxy silently creates a mock, so tests pass but may not exercise real API methods.A type-safe alternative:
import type { OpenChoreoClientApi } from '@openchoreo/backstage-plugin'; export type MockOpenChoreoClient = { [K in keyof OpenChoreoClientApi]: jest.Mock; };This preserves the Proxy's dynamic behavior at runtime while catching typos at compile time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts` at line 24, The MockOpenChoreoClient type is too permissive; change MockOpenChoreoClient to be keyed by the OpenChoreoClientApi interface so TypeScript enforces valid method names. Import OpenChoreoClientApi from '@openchoreo/backstage-plugin' and redefine MockOpenChoreoClient as a mapped type over keyof OpenChoreoClientApi producing jest.Mock for each key (maintaining the Proxy-based runtime behavior), and update any places that reference MockOpenChoreoClient accordingly.
36-43: Guard against well-known properties to prevent thenable/Promise confusion.The Proxy returns
jest.fn()for any accessed property, includingthen. This causes the mock to be mistaken for a thenable:const mockClient = createMockOpenChoreoClient(); await mockClient; // Hangs or fails - mockClient.then() is called as a thenable Promise.resolve(mockClient); // Wraps incorrectlyAdd guards for properties that should return
undefined:Proposed fix
+const PASSTHROUGH_PROPS = new Set([ + 'then', + 'toJSON', + 'asymmetricMatch', + '$$typeof', + 'nodeType', + 'tagName', +]); + return new Proxy(mocks, { - get(target, prop: string) { + get(target, prop) { + if (typeof prop === 'symbol' || PASSTHROUGH_PROPS.has(prop)) { + return undefined; + } if (!(prop in target)) { target[prop] = jest.fn(); } return target[prop]; }, }) as MockOpenChoreoClient;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts` around lines 36 - 43, The Proxy get trap in createMockOpenChoreoClient currently creates jest.fn() for any property (target, prop), which makes the mock behave like a thenable when accessing "then" or other well-known properties; update the get trap in packages/test-utils/src/frontend/mockOpenChoreoClient.ts to explicitly guard and return undefined for well-known keys (at minimum "then", "catch", "finally", Symbol.toStringTag, Symbol.iterator/Symbol.asyncIterator, and "constructor") before creating jest.fn(), so that accessing those props does not treat the mock as a Promise/thenable while preserving the existing behavior for other mocked methods on the mocks object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/openchoreo/src/components/Environments/components/SetupCard.tsx`:
- Around line 45-69: Update the getComponentDetails return type in the
OpenChoreoClientApi interface to include autoDeploy?: boolean so the method
signature matches the backend schema and test mocks; locate the
getComponentDetails declaration and add the optional autoDeploy property to the
returned Promise shape (e.g., include autoDeploy?: boolean alongside uid?,
deletionTimestamp?, parameters?) so places checking 'autoDeploy' in
componentData (and tests) are type-safe.
---
Nitpick comments:
In `@packages/test-utils/src/frontend/mockOpenChoreoClient.ts`:
- Line 24: The MockOpenChoreoClient type is too permissive; change
MockOpenChoreoClient to be keyed by the OpenChoreoClientApi interface so
TypeScript enforces valid method names. Import OpenChoreoClientApi from
'@openchoreo/backstage-plugin' and redefine MockOpenChoreoClient as a mapped
type over keyof OpenChoreoClientApi producing jest.Mock for each key
(maintaining the Proxy-based runtime behavior), and update any places that
reference MockOpenChoreoClient accordingly.
- Around line 36-43: The Proxy get trap in createMockOpenChoreoClient currently
creates jest.fn() for any property (target, prop), which makes the mock behave
like a thenable when accessing "then" or other well-known properties; update the
get trap in packages/test-utils/src/frontend/mockOpenChoreoClient.ts to
explicitly guard and return undefined for well-known keys (at minimum "then",
"catch", "finally", Symbol.toStringTag, Symbol.iterator/Symbol.asyncIterator,
and "constructor") before creating jest.fn(), so that accessing those props does
not treat the mock as a Promise/thenable while preserving the existing behavior
for other mocked methods on the mocks object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1f18d54-5b3f-4e1f-b1a1-a7c8e5aad9f5
📒 Files selected for processing (4)
packages/test-utils/src/frontend/mockOpenChoreoClient.tsplugins/openchoreo/src/components/Environments/components/SetupCard.tsxplugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsxplugins/openchoreo/src/components/Workflows/OverviewCard/useWorkflowsSummary.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/openchoreo/src/components/GitSecrets/hooks/useGitSecrets.test.tsx
…lientApi - Introduced an optional autoDeploy property in both OpenChoreoClient and OpenChoreoClientApi interfaces. - Updated SetupCard component to utilize the new autoDeploy property for improved state management. Signed-off-by: Stefinie Fernando <minolispencer@gmail.com>
LakshanSS
approved these changes
Apr 3, 2026
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
2 participants
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.
Purpose
Add unit tests for frontend components and improve code coverage
Approach
This pull request introduces comprehensive frontend testing utilities and adds unit tests for several OpenChoreo CI plugin components. It provides new helper functions for mocking Backstage entities and the OpenChoreo API client, and documents best practices for testing Backstage plugins. The main themes are improved testability and developer experience for frontend plugin development.
Frontend testing utilities and documentation:
TESTING_GUIDE.mdwith guidelines and best practices for testing frontend Backstage plugins, including when to userenderInTestApp,EntityProvider, and mocking strategies.mockComponentEntityandmockSystemEntityhelpers inentityFixtures.tsfor creating minimal Backstage entities in tests.createMockOpenChoreoClientutility to generate a fully mocked OpenChoreo API client for use in tests, with all methods stubbed asjest.fn().packages/test-utilsfor easy import in test files. [1] [2]@backstage/catalog-modelfor entity typing and made@openchoreo/test-utilsavailable to plugins. [1] [2]New Test Files Added
Traces (4 files, 38 tests)
Incidents (6 files, 52 tests)
RCA Reports (4 files, 28 tests)
Alerts (5 files, 50 tests)
Metrics (3 files, 39 tests)
Runtime Logs (6 files, 84 tests)
Refactored Tests (8 page-level files)
All page-level tests refactored to follow Backstage recommended testing patterns:
@backstage/core-componentswith real components viarenderInTestAppuseEntitywithEntityProviderfrom@backstage/plugin-catalog-reactSummary by CodeRabbit
Documentation
New Features
Tests