feat: add tests for dynamic parameters #18679
Conversation
jaaydenh
force-pushed
the
jaaydenh/dynamic-parameters-tests
branch
from
82e1666 to
3d3ccc3
Compare
Kira-Pilot
requested review from
a team and
BrunoQuaresma
and removed request for
Emyrk and
a team
| @@ -276,7 +276,7 @@ const CreateWorkspacePageExperimental: FC = () => { | |||
| <Helmet> | |||
| <title>{pageTitle(title)}</title> | |||
| </Helmet> | |||
| {!latestResponse || | |||
| {(!latestResponse && !wsError) || | |||
There was a problem hiding this comment.
This is an actual fix, right? Also, should we not guard against rendering if either one of these conditions is missing (regardless of whether they're missing together)?
Lastly, this boolean logic is getting difficult to reason about; I wonder if it's time to pull into a distinct boolean.
| readyState: number; | ||
| }; | ||
|
|
||
| function createMockWebSocket( |
| ], | ||
| }; | ||
|
|
||
| const renderCreateWorkspacePageExperimental = ( |
There was a problem hiding this comment.
WDYT about inlining this render function inside the CreateWorkspacePageExperimental describe block as it needs the websocket implementation to function? Otherwise we could just leave a comment. My worry is someone will grab it and not realize they're missing the websocket mock.
| callbacks.onClose(); | ||
| }); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
To avoid a setTimeout, what if we let the tests send events explicitly when they're ready using publisher?
|
I think a 1000+ LOC testing PR is difficult to review thoroughly. Your websocket mock could be a PR by itself, as could your fixtures in the |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestSuite
participant MockWebSocket
participant CreateWorkspacePageExperimental
User->>TestSuite: Run test
TestSuite->>MockWebSocket: Create mock connection
MockWebSocket->>CreateWorkspacePageExperimental: Simulate open event
CreateWorkspacePageExperimental->>MockWebSocket: Listen for messages
MockWebSocket->>CreateWorkspacePageExperimental: Send dynamic parameter data
User->>CreateWorkspacePageExperimental: Interact with form fields
CreateWorkspacePageExperimental->>MockWebSocket: Send parameter updates
MockWebSocket->>CreateWorkspacePageExperimental: Simulate error/close events
CreateWorkspacePageExperimental->>TestSuite: Render error or loader based on wsError/latestResponse
User->>CreateWorkspacePageExperimental: Submit form
CreateWorkspacePageExperimental->>TestSuite: Navigate or display errors
Estimated code review effort4 (~90 minutes) Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.tsx (1)
279-279: Consider extracting the complex boolean logic for better readability.The condition
(!latestResponse && !wsError)prevents the loader from showing when there's a WebSocket error, which is correct behavior. However, the boolean logic in this conditional is becoming complex and harder to reason about.Consider extracting this into a well-named boolean:
+ const shouldShowLoader = !latestResponse && !wsError && ( + !templateQuery.data || + isLoadingFormData || + isLoadingExternalAuth || + autoCreateReady + ); + - {(!latestResponse && !wsError) || - !templateQuery.data || - isLoadingFormData || - isLoadingExternalAuth || - autoCreateReady ? ( + {shouldShowLoader ? ( <Loader />site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx (2)
192-206: Consider inlining the render function or adding a warning comment.As mentioned in the previous review, this render function requires the WebSocket mock to be properly configured. Consider either moving it inside the describe block or adding a clear comment about the WebSocket dependency.
237-244: Replace setTimeout with explicit event publishing to fix test failures.The current setTimeout approach is causing test failures in the pipeline. The WebSocket messages aren't being delivered before the tests try to find the combobox elements.
Consider refactoring to allow tests to control when events are published:
-setTimeout(() => { - publisher.publishOpen(new Event("open")); - publisher.publishMessage( - new MessageEvent("message", { - data: JSON.stringify(mockDynamicParametersResponse), - }), - ); -}, 10); +// Store publisher for test access +callbacks.publisher = pub; +// Let tests explicitly trigger events when readyThen in tests:
await waitForLoaderToBeRemoved(); // Explicitly publish events publisher.publishOpen(new Event("open")); publisher.publishMessage(...); // Now wait for UI updates await waitFor(() => { expect(screen.getByRole("combobox", { name: /instance type/i })).toBeInTheDocument(); });
🧹 Nitpick comments (1)
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx (1)
566-674: Consider splitting this validation test into smaller, focused tests.This test is quite long and tests multiple aspects. Consider breaking it into separate tests:
- One for initial parameter display
- One for input validation triggering
- One for error message display
Example structure:
it("displays initial validation parameter with valid value", async () => { // Test initial rendering }); it("triggers validation on invalid input", async () => { // Test validation triggering }); it("displays validation error messages with proper styling", async () => { // Test error display });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
site/src/components/MultiSelectCombobox/MultiSelectCombobox.tsx(3 hunks)site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx(1 hunks)site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx(1 hunks)site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.tsx(1 hunks)site/src/testHelpers/entities.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to site/src/**/*.test.{ts,tsx,js,jsx} : React components and pages are organized in the `site/src/` directory, with Jest used for testing.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderdenttest/**/* : Enterprise features have dedicated test utilities in the `coderdenttest` package.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.tsx (3)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Effects run only on the client, never during server rendering.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Avoid reading or mutating refs during render; access them in event handlers or Effects after commit.
site/src/components/MultiSelectCombobox/MultiSelectCombobox.tsx (3)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Never mutate props, state, or values returned by Hooks. Always create new objects or use the setter from useState.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Every sibling element in a list needs a stable, unique key prop. Never use array indexes or Math.random(); prefer data-driven IDs.
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx (2)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : MUI components are deprecated - migrate away from these when encountered
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx (4)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to site/src/**/*.test.{ts,tsx,js,jsx} : React components and pages are organized in the site/src/ directory, with Jest used for testing.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.tsx : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.{ts,tsx} : Use ES modules (import/export) syntax, not CommonJS (require)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to site/**/*.ts : All user-facing frontend code is developed in TypeScript using React and lives in the site/ directory.
🪛 GitHub Actions: ci
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx
[error] 289-292: Jest test failure: Unable to find an accessible element with the role "combobox" and name matching /instance type/i. This caused the test 'CreateWorkspacePageExperimental › WebSocket Integration › sends parameter updates via WebSocket when form values change' to fail.
[error] 436-439: Jest test failure: Unable to find role="combobox" and name matching /instance type/i. This caused the test 'CreateWorkspacePageExperimental › Dynamic Parameter Types › renders dropdown parameter with options' to fail.
🔇 Additional comments (10)
site/src/components/MultiSelectCombobox/MultiSelectCombobox.tsx (1)
107-108: LGTM! Clean testability enhancement.The addition of the optional
data-testidprop follows best practices and provides proper test support without breaking existing functionality.Also applies to: 210-210, 460-460
site/src/modules/workspaces/DynamicParameter/DynamicParameter.tsx (1)
509-512: LGTM! Good testability improvements.The changes improve code clarity by using explicit property syntax and add proper test identification. The
data-testidpattern using the parameter name ensures unique test selectors.site/src/testHelpers/entities.ts (6)
3015-3026: LGTM: Enhanced MockPreviewParameter with comprehensive fieldsThe addition of the missing fields (
mutable,ephemeral,required,default_value,options,validations,icon,styling,order) makes this mock more complete and aligned with the TypesGen.PreviewParameter interface. This enhancement improves test coverage for dynamic parameter functionality.
3028-3068: LGTM: Well-structured dropdown parameter mockThis mock provides comprehensive test data for dropdown parameters with realistic AWS EC2 instance types. The structure correctly includes options with proper metadata and styling configuration.
3070-3091: LGTM: Appropriate tag-select parameter mockThe mock correctly uses
list(string)type with empty array defaults, which is appropriate for tag inputs. The configuration properly represents tag-select parameter behavior for testing.
3093-3114: LGTM: Proper switch parameter mockThe mock correctly represents a boolean switch parameter with string values ("true") which is appropriate for form handling. The configuration is realistic and suitable for testing switch functionality.
3116-3137: LGTM: Well-designed slider parameter mockThe mock correctly represents a numeric slider parameter with string value representation ("2") which is appropriate for form handling. The CPU count use case is realistic and the configuration is suitable for testing slider functionality.
3139-3208: LGTM: Comprehensive multi-select and validation parameter mocksBoth mocks are well-structured:
mockMultiSelectParameter: Provides realistic IDE options for testing multi-select functionalityvalidationParameter: Includes proper validation constraints (min/max) essential for testing form validation scenariosThese mocks enhance test coverage for complex parameter types and validation scenarios.
site/src/pages/CreateWorkspacePage/CreateWorkspacePageExperimental.test.tsx (2)
25-35: Well-structured type definitions for WebSocket mocking.The type definitions provide a clean interface for simulating WebSocket behavior in tests, with appropriate use of TypeScript features.
855-883: Excellent test coverage for navigation flow.The navigation test properly verifies the complete user journey from form submission to workspace navigation.
Summary by CodeRabbit
New Features
Bug Fixes
Tests