test(workspace):Automation script for Renaming and removing the Workspace#8356
test(workspace):Automation script for Renaming and removing the Workspace#8356guru-sharan-bruno wants to merge 9 commits into
Conversation
guru-sharan-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
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:
WalkthroughThe workspace action tests now use shared setup, cover terminal, rename, and remove flows, and add open-workspace coverage for dialog selection and cancel behavior. The ManageWorkspace actions button also gains a test id. ChangesWorkspace E2E Coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workspace/manage-workspace/manage-workspace.spec.ts`:
- Around line 95-97: The workspace action checks in manage-workspace.spec.ts are
order-dependent and should be scoped to the matching row instead of using
generic buttons or nth-child selectors. Update the interactions around the
workspace actions flow to locate the row by workspace name first, then click
that row’s actions menu and the Rename/Remove options from there. Replace the
`.workspace-item:nth-child(2)`-style assertions with name-based locators for
`New Workspace Name` and `Remove My Workspace`, and verify the removed workspace
is absent by name rather than by position.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 804a63c1-7ded-4181-97a5-81642d14b938
📒 Files selected for processing (1)
tests/workspace/manage-workspace/manage-workspace.spec.ts
guru-sharan-bruno
changed the title
| }); | ||
| } finally { | ||
| await closeElectronApp(app); | ||
| } |
There was a problem hiding this comment.
No need of try and final move the codeblock to test.beforeEach or test.afterEach
| const page = await app.firstWindow(); | ||
|
|
||
| try { | ||
| await page.locator('[data-app-state="loaded"]').waitFor({ timeout: 30000 }); |
There was a problem hiding this comment.
remove timeout update in all other places
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/workspace/manage-workspace/manage-workspace.spec.ts (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest titles read as instructions, not behaviour. Titles like
Click on the 3 dots icon on the workspace you want to rename.describe steps rather than the expected outcome. Prefer outcome-focused names, e.g.should rename a workspace/should remove a workspace, and move the step prose into the existingtest.stepblocks.Also applies to: 83-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/workspace/manage-workspace/manage-workspace.spec.ts` at line 55, The test titles in manage-workspace.spec.ts are written as user actions instead of expected behavior, so rename the affected test cases to outcome-focused descriptions. Update the relevant test declarations in the manage-workspace suite to use names like “should rename a workspace” or “should remove a workspace,” and keep the step-by-step interaction text inside the existing test.step blocks rather than in the test title.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workspace/manage-workspace/manage-workspace.spec.ts`:
- Line 19: The spec has an unused temporary-directory variable in the
manage-workspace test setup, which leaves misleading setup code behind. Remove
the wsLocation declaration from the workspace management spec and keep the test
focused on the values actually used; if the temp dir is still needed elsewhere,
reference the existing helper call through the relevant test setup identifiers
rather than storing an unused variable.
---
Nitpick comments:
In `@tests/workspace/manage-workspace/manage-workspace.spec.ts`:
- Line 55: The test titles in manage-workspace.spec.ts are written as user
actions instead of expected behavior, so rename the affected test cases to
outcome-focused descriptions. Update the relevant test declarations in the
manage-workspace suite to use names like “should rename a workspace” or “should
remove a workspace,” and keep the step-by-step interaction text inside the
existing test.step blocks rather than in the test title.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f48b44e-d986-4498-8d67-a7f1966b76b6
📒 Files selected for processing (2)
packages/bruno-app/src/components/ManageWorkspace/index.jstests/workspace/manage-workspace/manage-workspace.spec.ts
| test('should open terminal from the workspace actions menu', async ({ launchElectronApp, createTmpDir }) => { | ||
| const wsLocation = await createTmpDir('ws-location-terminal'); | ||
| test.describe('Manage Workspace Actions - Open in Terminal', () => { | ||
| test('should open terminal from the workspace actions menu', async ({ page }) => { |
There was a problem hiding this comment.
Sanity tag is not added here. Do we not need it?
| import type { ElectronApplication } from '@playwright/test'; | ||
| import { expect, test } from '../../../playwright'; | ||
| import { buildCommonLocators, waitForReadyPage } from '../../utils/page'; | ||
| import fs from 'fs/promises'; |
There was a problem hiding this comment.
remove this file changes add a separate PR for this.
| && fs.existsSync(path.join(fullPath, 'workspace.yml')) | ||
| ); | ||
| await test.step('Open Manage Workspaces', async () => { | ||
| await page.locator('.workspace-name-container').click(); |
There was a problem hiding this comment.
Please use data-testid
| myWorkspaceDropdownItem: () => page.locator('.dropdown-item'), | ||
| manageWorkspaceTitle: () => page.getByText('Manage Workspace', { exact: true }), | ||
| defaultWorkspaceItem: () => page.locator('.workspace-item'), | ||
| moreActionsBtn: () => page.locator('.more-actions-btn'), |
There was a problem hiding this comment.
can we use datatest-id
| submitRemoveBtn: () => page.getByTestId('modal-submit-btn') | ||
| }); | ||
|
|
||
| export const manageWorkspace = async (page: Page) => { |
There was a problem hiding this comment.
this function name can be more specific like openManageWorkspace or something else.
| import { buildTitleBarLocators } from '../title-bar'; | ||
|
|
||
| export const buildManageWorkspaceLocators = (page: Page) => ({ | ||
| myWorkspaceDropdownItem: () => page.locator('.dropdown-item'), |
There was a problem hiding this comment.
locaters structure and name can be improved.
export const buildManageWorkspaceLocators = (page: Page) => ({
// The Manage Workspace panel heading.
manageWorkspaceTitle: () => page.getByText('Manage Workspace', { exact: true }),
// All workspace rows, or a single row matched by name.
workspaceItem: (name?: string) => {
const rows = page.locator('.workspace-item');
return name ? rows.filter({ hasText: name }) : rows;
},
// The "..." actions button, scoped to a given workspace row.
moreActionsBtn: (row: Locator) => row.getByTestId('more-actions-btn'),
// Rename modal's name field.
workspaceNameInput: () => page.locator('#workspace-name'),
// The active workspace name shown in the title bar (updates after a rename).
activeWorkspaceName: () => buildTitleBarLocators(page).activeWorkspaceName(),
// First terminal session entry opened via "Open in Terminal".
terminalSession: () => page.getByTestId('session-list-0')
})
There was a problem hiding this comment.
Further minor enhancement
- use plural =>
workspaceItems: (name?: string) => { - enhance =>
terminalSession: (index: number = 0) => page.getByTestId(`session-list-${index}`)
| await clickOnMoreActionsButton(page, 'Custom Workspace'); | ||
| await renameOrRemoveWorkspaceOnPopup(page, 'Rename'); | ||
| await enterNewWorkspaceFilename(page, 'New Workspace Name', 'Rename'); | ||
| await expect(locators.renamedWorkspaceItem()).toHaveText('New Workspace Name'); |
There was a problem hiding this comment.
This renamedWorkspaceItem does not make sence we can just name it activeWorkspaveName
There was a problem hiding this comment.
We can rename non-active workspaces too. Fix accordingly.
| }); | ||
| }; | ||
|
|
||
| export const clickOnMoreActionsButton = async (page: Page, workspaceName: string) => { |
There was a problem hiding this comment.
can we rename it to openWorkspaceActionsMenu?
| }); | ||
| }; | ||
|
|
||
| export const clickOnRemoveButtonOnPopup = async (page: Page) => { |
There was a problem hiding this comment.
please rename it to confirmRemoveWorkspace.
| }); | ||
| }; | ||
|
|
||
| export const enterNewWorkspaceFilename = async (page: Page, workspaceName: string, btnName: string) => { |
There was a problem hiding this comment.
Please change the name to enterNewWorkspaceName
| }); | ||
| }; | ||
|
|
||
| export const renameOrRemoveWorkspaceOnPopup = async (page: Page, optionName: string) => { |
There was a problem hiding this comment.
change it to selectWorkspaceAction. More generic, and won't need renaming when new menu actions are added.
| export const buildManageWorkspaceLocators = (page: Page) => ({ | ||
| myWorkspaceDropdownItem: () => page.locator('.dropdown-item'), | ||
| manageWorkspaceTitle: () => page.getByText('Manage Workspace', { exact: true }), | ||
| defaultWorkspaceItem: () => page.locator('.workspace-item'), |
There was a problem hiding this comment.
This is not reliable. page.locator('.workspace-item') is not always default item
| submitRenameBtn: () => page.getByTestId('modal-submit-btn'), | ||
| renamedWorkspaceItem: () => page.getByTestId('workspace-name'), | ||
| submitRemoveBtn: () => page.getByTestId('modal-submit-btn') |
There was a problem hiding this comment.
submitRenameBtn, submitRemoveBtn are both relying on the default testid from the modal component.
I suggest adding proper testid for the modals and use them.
// rename modal
<Modal
data-testid="rename-workspace-modal"
...
// usage becomes
submitRemoveBtn: () => page.getByTestId('rename-workspace-modal-submit-btn')
| import { buildTitleBarLocators } from '../title-bar'; | ||
|
|
||
| export const buildManageWorkspaceLocators = (page: Page) => ({ | ||
| myWorkspaceDropdownItem: () => page.locator('.dropdown-item'), |
There was a problem hiding this comment.
myWorkspaceDropdownItem: () => page.locator('.dropdown-item'),should be part oftests/utils/page/title-bar.ts- rename
myWorkspaceDropdownItem=>importWorkspaceOptionrefer:importWorkspaceOption - We can also add a dynamic option selector
workspaceMenuOption: (name: string) => page.getByTestId('workspace-menu').locator('.dropdown-item');
|
|
||
| export const renameOrRemoveWorkspaceOnPopup = async (page: Page, optionName: string) => { | ||
| const locators = buildManageWorkspaceLocators(page); | ||
| await test.step('Rename or remove workspace on popup', async () => { |
There was a problem hiding this comment.
step description is not accurate to action performed
|
|
||
| export const clickOnRemoveButtonOnPopup = async (page: Page) => { | ||
| const locators = buildManageWorkspaceLocators(page); | ||
| await test.step('Verify the workspace name is updated in the workspace list.', async () => { |
There was a problem hiding this comment.
Step description is not accurate to action performed. Actions cannot have verify steps.
| await createWorkspace(page, 'Custom Workspace'); | ||
| }); | ||
|
|
||
| await test.step('Open manage workspaces', async () => { |
There was a problem hiding this comment.
Repeated/Duplicate step.
await manageWorkspace(page); internally have the same step.
| await test.step('Open manage workspaces', async () => { | ||
| const locators = buildManageWorkspaceLocators(page); | ||
| await manageWorkspace(page); | ||
| await expect(locators.manageWorkspaceTitle()).toBeVisible(); |
There was a problem hiding this comment.
beforeEach block cannot have test assertions.
use .waitFor({ state: 'visible' }) instead, if this is added as a dynamic wait logic.
| await expect(defaultWorkspaceItem.locator('.more-actions-btn')).toHaveCount(0); | ||
| }); | ||
| test.describe('Manage workspace actions-open in terminal', () => { | ||
| test('TC-3109: Verify that the workspace should open terminal from the workspace actions menu', { tag: '@sanity' }, async ({ page }) => { |
There was a problem hiding this comment.
Descriptions can be improved.
| await clickOnMoreActionsButton(page, 'Custom Workspace'); | ||
| await renameOrRemoveWorkspaceOnPopup(page, 'Rename'); | ||
| await enterNewWorkspaceFilename(page, 'New Workspace Name', 'Rename'); | ||
| await expect(locators.renamedWorkspaceItem()).toHaveText('New Workspace Name'); |
There was a problem hiding this comment.
We can rename non-active workspaces too. Fix accordingly.
There was a problem hiding this comment.
All tests are passing when run in parallel with multiple workers. But when run using single worker (npm run test:e2e tests/workspace/manage-workspace/manage-workspace.spec.ts -- --workers=1) it fails due to stale state from the previous test case is not cleaned up.
4 participants
JIRA: https://usebruno.atlassian.net/browse/BRU-3576
Description
This JIRA ticket is about adding the sanity automation script for Open workspace and manage workspace.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Summary by CodeRabbit