test(deployment): add e2e tests for SDL builder #1995
Conversation
- Add 10 new e2e test cases covering SDL builder deployment flow - Extend BuildTemplatePage with helper methods for form interactions - Tests cover: image input, preview, multi-service, validation, reset Tests verify: - Basic SDL creation and preview functionality - Deployment flow up to wallet connection - Multi-service configuration - Form validation and reset behavior - YAML structure generation
WalkthroughAdds Playwright page-object methods and locators to BuildTemplatePage for interacting with the Build Template UI and introduces a new UI test suite exercising preview, deploy, add-service, reset, and YAML/preview validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Playwright
participant BuildTemplatePage
participant SDL_UI
participant PreviewModal
participant WalletDialog
Tester->>Playwright: run sdl-builder-deployment.spec
Playwright->>SDL_UI: navigate to /sdl-builder
Playwright->>BuildTemplatePage: new BuildTemplatePage(page)
Note right of BuildTemplatePage: Image entry & preview
Tester->>BuildTemplatePage: fillImageName("nginx:latest")
BuildTemplatePage->>SDL_UI: type into image input (getImageNameInput)
Tester->>BuildTemplatePage: clickPreview()
BuildTemplatePage->>SDL_UI: click preview button (getPreviewButton)
SDL_UI-->>PreviewModal: render SDL preview
PreviewModal-->>BuildTemplatePage: expose preview text (getPreviewTextLocator)
Tester->>BuildTemplatePage: closePreview()
Note right of BuildTemplatePage: Deploy flow
Tester->>BuildTemplatePage: clickDeploy()
BuildTemplatePage->>SDL_UI: click deploy button (getDeployButton)
SDL_UI-->>WalletDialog: open wallet connect dialog
Note right of BuildTemplatePage: Add service (async wait)
Tester->>BuildTemplatePage: addService()
BuildTemplatePage->>SDL_UI: click add service (getAddServiceButton)
BuildTemplatePage->>BuildTemplatePage: waitForServiceAdded("service-2", timeout=10000)
Tester->>BuildTemplatePage: clickReset()
BuildTemplatePage->>SDL_UI: clear form fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts (4)
40-54: Replace arbitrary timeout with state-based wait.Line 48 uses
page.waitForTimeout(1000), which is a Playwright anti-pattern that makes tests fragile and slower. Wait for a specific UI state instead.Replace the timeout with a wait for the new service to be visible:
await sdlBuilderPage.addService(); -await page.waitForTimeout(1000); +// Wait for the new service element to be added to the DOM +await expect(page.getByTestId("service-2-container").or(page.getByText("service-2"))).toBeVisible(); await sdlBuilderPage.clickPreview();If you don't have a specific test ID for the second service, wait for any element that indicates the service was added.
56-75: Replace arbitrary timeouts with state-based waits.Lines 64 and 68 use
page.waitForTimeout(), which is a Playwright anti-pattern. Replace these with waits for specific UI states.Apply these changes:
await sdlBuilderPage.addService(); -await page.waitForTimeout(500); +// Wait for service to be added +await expect(page.getByTestId("service-2-container").or(page.getByText("service-2"))).toBeVisible(); await sdlBuilderPage.clickReset(); -await page.waitForTimeout(1000); +// Wait for form to reset - could wait for a loading indicator to disappear +// or for the form to return to initial state +await expect(page.getByTestId("image-name-input")).toHaveValue(""); const imageInput = page.getByTestId("image-name-input"); await expect(imageInput).toBeVisible();
77-92: Simplify negative assertion logic.Lines 89-91 use
.catch(() => true)to handle the case where the wallet button is not visible. This pattern is unclear and swallows errors. Use a straightforward negative assertion instead.Apply this diff:
const walletBtn = page.getByTestId("connect-wallet-btn"); -await expect(walletBtn) - .not.toBeVisible({ timeout: 2000 }) - .catch(() => true); +await expect(walletBtn).not.toBeVisible();Playwright's
not.toBeVisible()already handles the case where the element doesn't exist or is hidden, so no error handling is needed.
124-141: Replace arbitrary timeouts with state-based waits.Lines 131 and 135 use
page.waitForTimeout(), which is a Playwright anti-pattern. Replace these with waits for specific UI state changes.Apply these changes:
await sdlBuilderPage.addService(); -await page.waitForTimeout(500); +// Wait for second service to appear +await expect(page.getByTestId("service-2-container").or(page.getByText("service-2"))).toBeVisible(); await sdlBuilderPage.clickPreview(); -await page.waitForTimeout(1000); +// Wait for preview modal to fully render +await expect(page.getByText(/service-1:/).first()).toBeVisible(); await expect(page.getByText(/service-1:/).first()).toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx(1 hunks)apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsxapps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsxapps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
🧬 Code graph analysis (2)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (3)
apps/deploy-web/tests/ui/pages/DeployBasePage.tsx (1)
DeployBasePage(7-88)apps/deploy-web/tests/ui/fixture/test-env.config.ts (1)
testEnvConfig(12-16)apps/deploy-web/tests/ui/fixture/context-with-extension.ts (1)
expect(77-77)
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts (1)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (1)
BuildTemplatePage(6-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (15)
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts (6)
1-4: LGTM!The imports and test suite structure follow standard Playwright conventions.
14-27: LGTM!The test logic properly verifies image name input and SDL preview functionality with appropriate assertions.
29-38: LGTM!The test correctly verifies the deployment flow up to wallet connection. The explicit timeout on line 37 is reasonable given wallet interactions can be slower.
94-106: LGTM!The test effectively verifies SDL preview functionality with multiple images. The loop-based approach provides good coverage without code duplication.
108-122: LGTM!The test properly verifies the essential YAML structure components (version, services, profiles, deployment) in the generated SDL.
143-152: LGTM!The test concisely verifies that the preview button is enabled when a valid image is present.
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (9)
1-5: LGTM!The imports are correct and properly organized.
7-10: LGTM!The navigation method correctly overrides the parent implementation to reach the SDL Builder page.
12-14: LGTM!The method properly encapsulates the image name input interaction.
16-18: LGTM!The method uses accessible role-based selectors with case-insensitive matching, which is a Playwright best practice.
20-22: LGTM!The regex anchors (
^deploy$) correctly ensure an exact match to avoid clicking buttons with similar text like "deployment".
24-30: LGTM!Both methods follow consistent patterns with appropriate selectors.
32-34: LGTM!The method correctly uses
.first()to handle potential multiple matches in the SDL preview content.
36-38: LGTM!The method appropriately uses
.first()to click the close button, handling scenarios where multiple close buttons might exist in the page.
40-42: LGTM!The method properly verifies the input value using the appropriate
toHaveValueassertion.
| } | ||
|
|
||
| async verifyImageNameValue(value: string) { | ||
| await expect(this.page.getByTestId("image-name-input")).toHaveValue(value); |
There was a problem hiding this comment.
issue(blocking): assertions should not be part of Page Objects. Page objects work with the page state, expose it or allow to change it and then test verifies the correctness of the page state
There was a problem hiding this comment.
Removed all assertions from the Page Object: replaced them with getter methods
| await expect(page.getByRole("button", { name: /^deploy$/i })).toBeVisible(); | ||
| await expect(page.getByRole("button", { name: /preview/i })).toBeVisible(); | ||
| await expect(page.getByRole("button", { name: /add service/i })).toBeVisible(); |
There was a problem hiding this comment.
issue(blocking): better to abstract this thing, especially that you use also use them in Page Object implementation
There was a problem hiding this comment.
Abstracted locator selection by using Page Object getter methods
| await sdlBuilderPage.verifyPreviewSdlContains("nginx:latest"); | ||
| await sdlBuilderPage.verifyPreviewSdlContains("version:"); | ||
| await sdlBuilderPage.verifyPreviewSdlContains("services:"); |
There was a problem hiding this comment.
suggestion(non-blocking): I think would be nice to use js-yaml to load yaml and check values based on yaml. This additionally ensures that preview builds the correct yaml
There was a problem hiding this comment.
wouldn't that be a unit test like logic?
|
|
||
| await sdlBuilderPage.addService(); | ||
|
|
||
| await page.waitForTimeout(1000); |
There was a problem hiding this comment.
issue(blocking): let's wait for the service to be added on the page with waitFor(...), this is more reliable than a pure timeout
| await sdlBuilderPage.closePreview(); | ||
| }); | ||
|
|
||
| test("reset form to default state", async ({ page, context }) => { |
There was a problem hiding this comment.
thought(non-blocking): I think this one can be tested with unit test. no need for e2e here
| await sdlBuilderPage.verifyImageNameValue("nginx:latest"); | ||
| }); | ||
|
|
||
| test("validation prevents deployment with empty image", async ({ page, context }) => { |
There was a problem hiding this comment.
suggestion(non-blocking): such validation is also should be done by unit tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (1)
30-32: Consider more specific selectors to avoid.first()fragility.Both methods use
.first()to select from potentially multiple matches, which can be fragile:
getPreviewTextLocator: If the same text appears elsewhere on the page, this returns the first match, which may not be from the preview dialog.closePreview: If multiple "close" buttons exist (e.g., modals, sidebars), this clicks the first one, which might not be the preview close button.Consider scoping selectors to the preview container:
-getPreviewTextLocator(text: string) { - return this.page.getByText(text).first(); +getPreviewTextLocator(text: string) { + return this.page.locator('[data-testid="preview-dialog"]').getByText(text); } async closePreview() { - await this.page.getByRole("button", { name: /close/i }).first().click(); + await this.page.locator('[data-testid="preview-dialog"]').getByRole("button", { name: /close/i }).click(); }Alternatively, use more specific test-ids for these elements if available.
Also applies to: 34-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx(1 hunks)apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx
🧬 Code graph analysis (1)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (2)
apps/deploy-web/tests/ui/pages/DeployBasePage.tsx (1)
DeployBasePage(7-88)apps/deploy-web/tests/ui/fixture/test-env.config.ts (1)
testEnvConfig(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (4)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (4)
5-8: LGTM!The navigation logic is clean and correctly overrides the parent method to access the SDL builder directly.
10-28: LGTM!The action methods use appropriate selectors and follow Playwright best practices. The use of role-based selectors with regex patterns is correct, and
clickDeployproperly uses anchored regex to avoid matching similar button names.
38-52: LGTM!The accessor methods properly return locators for test assertions, following the page object pattern correctly. The use of both test-id and role-based selectors is appropriate.
58-60: LGTM, pending fix togetServiceLocator.The wait logic is correctly implemented. Once the regex escaping issue in
getServiceLocator(lines 54-56) is resolved, this method will function reliably.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (1)
30-32: Consider precision of text locator.Using
.first()on a text locator could be brittle if multiple elements contain the same text. If the preview displays duplicate text in different contexts, this might select the wrong element.If multiple matches are possible, consider using more specific locators or adding a container scope:
getPreviewTextLocator(text: string) { return this.page.locator('[data-testid="preview-modal"]').getByText(text).first(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx(1 hunks)apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.tsapps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.tsapps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts
🧬 Code graph analysis (2)
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts (2)
apps/deploy-web/tests/ui/fixture/context-with-extension.ts (1)
expect(77-77)apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (1)
BuildTemplatePage(4-66)
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (2)
apps/deploy-web/tests/ui/pages/DeployBasePage.tsx (1)
DeployBasePage(7-88)apps/deploy-web/tests/ui/fixture/test-env.config.ts (1)
testEnvConfig(12-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (3)
apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts (1)
97-106: LGTM! Setup function follows guidelines correctly.The setup function is properly positioned at the bottom of the describe block, accepts a single parameter with inline type definition, has no return type annotation, and avoids shared state. This follows the project's coding guidelines exactly.
apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx (2)
54-57: LGTM! Regex escaping implemented correctly.The service name is properly escaped before being used in the regular expression, preventing incorrect matches when service names contain regex metacharacters.
59-61: Verify filter precision for service name input.The
hasTextfilter might match unintended textboxes ifserviceNameappears in multiple locations. Consider whether a more specific selector (e.g., using container scope or test-id) would be more reliable.Please verify that this locator uniquely identifies the intended service name input field, especially when multiple services with similar names are present.
| async waitForServiceAdded(serviceName: string, timeout = 10000) { | ||
| await this.page.locator(`input[type="text"][value="${serviceName}"]`).first().waitFor({ state: "visible", timeout }); | ||
| } |
There was a problem hiding this comment.
Escape special characters in CSS attribute selector.
The serviceName is directly interpolated into a CSS attribute selector without escaping quotes or special characters. If serviceName contains quotes (e.g., service"1), the selector will break.
Apply this diff to escape the service name:
async waitForServiceAdded(serviceName: string, timeout = 10000) {
- await this.page.locator(`input[type="text"][value="${serviceName}"]`).first().waitFor({ state: "visible", timeout });
+ const escapedName = serviceName.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
+ await this.page.locator(`input[type="text"][value="${escapedName}"]`).first().waitFor({ state: "visible", timeout });
}Alternatively, use a more robust approach with filter:
async waitForServiceAdded(serviceName: string, timeout = 10000) {
- await this.page.locator(`input[type="text"][value="${serviceName}"]`).first().waitFor({ state: "visible", timeout });
+ await this.page.locator('input[type="text"]').filter({ hasText: serviceName }).first().waitFor({ state: "visible", timeout });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async waitForServiceAdded(serviceName: string, timeout = 10000) { | |
| await this.page.locator(`input[type="text"][value="${serviceName}"]`).first().waitFor({ state: "visible", timeout }); | |
| } | |
| async waitForServiceAdded(serviceName: string, timeout = 10000) { | |
| const escapedName = serviceName.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); | |
| await this.page.locator(`input[type="text"][value="${escapedName}"]`).first().waitFor({ state: "visible", timeout }); | |
| } |
🤖 Prompt for AI Agents
In apps/deploy-web/tests/ui/pages/BuildTemplatePage.tsx around lines 63 to 65,
the CSS attribute selector interpolates serviceName directly which breaks if
serviceName contains quotes or special characters; either escape serviceName for
use inside the attribute selector (replace backslashes and both single and
double quotes and other CSS-special characters) before interpolation, or avoid
attribute selectors entirely and instead locate inputs by type (e.g.,
input[type="text"]) and filter by their value/text content using Playwright's
filter/hasText or by reading element value and matching against serviceName;
update waitForServiceAdded to use one of these safer approaches and keep the
same timeout handling.
| test("preview button always available with valid image", async ({ page, context }) => { | ||
| const sdlBuilderPage = new BuildTemplatePage(context, page, "sdl-builder"); | ||
| await sdlBuilderPage.gotoInteractive(); | ||
|
|
||
| await sdlBuilderPage.fillImageName("alpine:latest"); | ||
|
|
||
| await expect(sdlBuilderPage.getPreviewButton()).toBeEnabled(); | ||
| }); |
There was a problem hiding this comment.
Use the setup function pattern consistently.
This test directly instantiates BuildTemplatePage instead of using the setup function that all other tests use. According to coding guidelines, all tests should use the setup function pattern.
Apply this diff to fix the inconsistency:
-test("preview button always available with valid image", async ({ page, context }) => {
- const sdlBuilderPage = new BuildTemplatePage(context, page, "sdl-builder");
- await sdlBuilderPage.gotoInteractive();
-
- await sdlBuilderPage.fillImageName("alpine:latest");
+test("preview button always available with valid image", async ({ page, context }) => {
+ const { sdlBuilderPage } = await setup({ page, context, imageName: "alpine:latest" });
await expect(sdlBuilderPage.getPreviewButton()).toBeEnabled();
});As per coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/deploy-web/tests/ui/sdl-builder-deployment.spec.ts around lines 88 to
95, the test instantiates BuildTemplatePage directly instead of using the shared
setup function; replace the direct construction with the standard setup pattern
(call the existing async setup("sdl-builder") helper and destructure the
returned objects), then use the returned sdlBuilderPage to call
gotoInteractive(), fillImageName("alpine:latest"), and assert the preview button
is enabled so the test matches other specs' setup usage.
Tests verify:
Summary by CodeRabbit