open-collection testcases#8381
Conversation
aman-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
WalkthroughAdds a new collection removal Playwright spec, updates shared collection test helpers and locators, and expands the multiple-collection opening test assertions. ChangesCollection test support updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
| await removeModal.removeButton().click(); | ||
| } | ||
|
|
||
| await removeModal.modal().waitFor({ state: 'hidden', timeout: 5000 }); |
There was a problem hiding this comment.
Do we need this timeout if not remove it
There was a problem hiding this comment.
Not really required. Added just for time when application might be slow. Removed.
| * secrets live on separate tabs, so a secret is routed to the Secrets tab and a | ||
| * plain variable to the Variables tab before the row is added. | ||
| * @param page - The page object | ||
| * @param page - The page object |
| await expect(locators.sidebar.folderRequest(parentName, requestName)).toBeVisible(); | ||
| } else { | ||
| await expect(locators.sidebar.request(requestName)).toBeVisible(); | ||
| await expect(locators.sidebar.scopedRequest(parentName, requestName)).toBeVisible(); |
There was a problem hiding this comment.
Please revert this line back to locators.sidebar.request(requestName).
Changing the post-create assertion to scopedRequest(parentName, requestName) breaks existing tests
There was a problem hiding this comment.
Reverted.
This was added to make the assertion more robust.
Incase we have multiple collection expanded and all collection have same request name then the "await expect(locators.sidebar.request(requestName)).toBeVisible();" will fail. Tried making it more robust.
| * @param page - The page object | ||
| * @returns void | ||
| */ | ||
| const closeAllCollections = async (page) => { |
There was a problem hiding this comment.
revert back changes done in closeAllCollections. don't touch old actions do changes if necessary.
There was a problem hiding this comment.
Reverted.
This change was made because I have added few locator in locator.ts and wanted to reuse it here also, to avoid any duplicate locators.I will not make any changes on current methods moving forward
| folder: (name: string) => page.locator('.collection-item-name').filter({ hasText: name }), | ||
| request: (name: string) => page.locator('.collection-item-name').filter({ hasText: name }), | ||
| folderRequest: (folderName: string, requestName: string) => { | ||
| const toCollectionSlug = (name: string) => name.replace(/\s+/g, '-').toLowerCase(); |
There was a problem hiding this comment.
Do we need this we are just using it one time right?
There was a problem hiding this comment.
Reverted.
This can be used in multiple places. for now just one usecase.
There was a problem hiding this comment.
added this inside cons collectionscope
| removeCollection: { | ||
| modal: removeCollectionModal, | ||
| path: () => removeCollectionModal().locator('.collection-path'), | ||
| removeButton: () => removeCollectionModal().getByRole('button', { name: 'Remove', exact: true }), | ||
| cancelButton: () => removeCollectionModal().getByRole('button', { name: 'Cancel', exact: true }), | ||
| discardAllAndRemoveButton: () => page.getByRole('button', { name: 'Discard All and Remove' }) | ||
| } | ||
| }, | ||
| toast: { | ||
| collectionRemovedFromWorkspace: () => page.getByText('Collection removed from workspace') |
There was a problem hiding this comment.
These items (remove collections) should not be part of generic modal element locators or toast locators.
Can be moved to own page.
| // The sidebar tree wraps each collection in `#collection-<slug>`; scope queries | ||
| // to it to disambiguate items that share names across collections. | ||
| collectionScope, | ||
| scopedRequest: (collectionName: string, requestName: string) => |
There was a problem hiding this comment.
unused locator added?
| const openCollectionActionsMenu = async (page: Page, collectionName: string) => { | ||
| await test.step(`Open actions menu for collection "${collectionName}"`, async () => { | ||
| const locators = buildCommonLocators(page); | ||
| await locators.sidebar.collectionRow(collectionName).hover(); | ||
| await locators.actions.collectionActions(collectionName).click(); | ||
| }); | ||
| }; | ||
|
|
||
| const clickRemoveInCollectionMenu = async (page: Page) => { | ||
| const locators = buildCommonLocators(page); | ||
| await locators.dropdown.item('Remove').click(); | ||
| await locators.modal.removeCollection.modal().waitFor({ state: 'visible', timeout: 5000 }); | ||
| }; | ||
|
|
||
| const confirmRemoveCollection = async (page: Page) => { |
There was a problem hiding this comment.
Above actions are generic enough to be moved to pages/<component>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/collection/open/open-multiple-collections.spec.ts`:
- Around line 77-80: The open-multiple-collections test step only clicks
collection locators without proving either collection became active. Update the
test around the collection1Element and collection2Element clicks to add a
user-visible assertion after each click, such as verifying the selected sidebar
state or the active collection pane/header, so the test checks behavior not just
clickability. Keep the assertions in the same test.step and use multiple
assertions to confirm both opened collections are visibly active.
- Around line 34-43: The open-multiple-collections spec is using hardcoded
collection names that can collide with persisted sidebar state across runs.
Update the test setup in open-multiple-collections.spec.ts so the collection
names created in the collection1Config and collection2Config blocks are derived
from each test’s isolated temp directory or another per-test unique value, and
make the same change for the additional collection name usage referenced later
in the spec. Keep the identifiers stable within a single test but unique per
test to ensure the locators in this scenario bind to the correct collections.
- Around line 64-75: The open-multiple-collections test is missing a pre-open
visibility check for collection2Element, so leaked state can slip through
unnoticed. In open-multiple-collections.spec.ts, update the test step around the
open action to assert both collection1Element and collection2Element are not
visible before clicking the Open collection flow, using the existing
collection1Element and collection2Element locators. Keep the post-action
visibility assertions, but make sure the test proves the command surfaced both
collections from a clean state.
In `@tests/utils/page/collection.ts`:
- Around line 48-60: The Confirm Remove Collection helper is checking for
discard-first instead of triggering the initial remove action, so the
draft-confirmation flow is never reached. Update buildCollectionLocators usage
in Confirm Remove Collection to click removeButton() first, then detect and
handle discardAllAndRemoveButton() if it appears, using options.forceDiscard
only for that follow-up click before waiting on removeModal().hide.
🪄 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: d6e7517d-bc67-4773-9835-f2b292634407
📒 Files selected for processing (5)
tests/collection/open/open-collection.spec.tstests/collection/open/open-multiple-collections.spec.tstests/utils/page/collection.tstests/utils/page/index.tstests/utils/page/locators.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/page/index.ts
| const collection1Config = { | ||
| version: '1', | ||
| name: 'Test Collection 1', | ||
| type: 'collection' | ||
| }; | ||
| // Create bruno.json for second collection | ||
| const collection2Config = { | ||
| version: '1', | ||
| name: 'Test Collection 2', | ||
| type: 'collection' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use per-test collection names here.
These fixed names make the text locators vulnerable to persisted/sidebar state from another test run, so this spec can bind to the wrong collection and pass for the wrong reason. Derive the names from the isolated temp dirs instead.
Minimal change
+ const collection1Name = path.basename(collection1Dir);
+ const collection2Name = path.basename(collection2Dir);
const collection1Config = {
version: '1',
- name: 'Test Collection 1',
+ name: collection1Name,
type: 'collection'
};
const collection2Config = {
version: '1',
- name: 'Test Collection 2',
+ name: collection2Name,
type: 'collection'
};
@@
- const collection1Element = page.locator('`#sidebar-collection-name`').getByText('Test Collection 1');
- const collection2Element = page.locator('`#sidebar-collection-name`').getByText('Test Collection 2');
+ const collection1Element = page.locator('`#sidebar-collection-name`').getByText(collection1Name);
+ const collection2Element = page.locator('`#sidebar-collection-name`').getByText(collection2Name);As per coding guidelines, "E2E tests must be parallel-safe... Each test gets isolated temp paths and unique workspace/project names." As per path instructions, "Each test gets isolated temp paths and unique workspace/project names."
Also applies to: 61-62
🤖 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/collection/open/open-multiple-collections.spec.ts` around lines 34 -
43, The open-multiple-collections spec is using hardcoded collection names that
can collide with persisted sidebar state across runs. Update the test setup in
open-multiple-collections.spec.ts so the collection names created in the
collection1Config and collection2Config blocks are derived from each test’s
isolated temp directory or another per-test unique value, and make the same
change for the additional collection name usage referenced later in the spec.
Keep the identifiers stable within a single test but unique per test to ensure
the locators in this scenario bind to the correct collections.
Sources: Coding guidelines, Path instructions
| await test.step('Initiate the simultaneous opening command', async () => { | ||
| await expect(collection1Element).not.toBeVisible(); | ||
|
|
||
| // Click on plus icon button and then "Open collection" in the dropdown | ||
| await page.getByTestId('collections-header-add-menu').click(); | ||
| await page.locator('.tippy-box .dropdown-item').filter({ hasText: 'Open collection' }).click(); | ||
| }); | ||
|
|
||
| await test.step('Verify the launch status of both collections', async () => { | ||
| await expect(collection1Element).toBeVisible(); | ||
| await expect(collection2Element).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Re-add the pre-open check for the second collection.
Right now leaked state for collection2Element goes unnoticed, and the later visibility assertion can still pass without proving the open action surfaced both collections.
As per coding guidelines, "Cover both the happy path and the realistically problematic paths." As per path instructions, "E2E tests must be parallel-safe. No shared user data directories... or global app state."
🤖 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/collection/open/open-multiple-collections.spec.ts` around lines 64 -
75, The open-multiple-collections test is missing a pre-open visibility check
for collection2Element, so leaked state can slip through unnoticed. In
open-multiple-collections.spec.ts, update the test step around the open action
to assert both collection1Element and collection2Element are not visible before
clicking the Open collection flow, using the existing collection1Element and
collection2Element locators. Keep the post-action visibility assertions, but
make sure the test proves the command surfaced both collections from a clean
state.
Sources: Coding guidelines, Path instructions
| await test.step('Check the functionality and accessibility of each opened collection', async () => { | ||
| await collection1Element.click(); | ||
| await collection2Element.click(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert a user-visible outcome after each click.
This step only proves the locators are clickable. It does not verify that either collection actually became active/opened. Please add an observable assertion after each click (for example, selected sidebar state or the active collection pane/header).
As per coding guidelines, "Tests must verify user-visible behaviour, not implementation details." As per path instructions, "Use multiple assertions."
🤖 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/collection/open/open-multiple-collections.spec.ts` around lines 77 -
80, The open-multiple-collections test step only clicks collection locators
without proving either collection became active. Update the test around the
collection1Element and collection2Element clicks to add a user-visible assertion
after each click, such as verifying the selected sidebar state or the active
collection pane/header, so the test checks behavior not just clickability. Keep
the assertions in the same test.step and use multiple assertions to confirm both
opened collections are visibly active.
Sources: Coding guidelines, Path instructions
| await test.step('Confirm Remove Collection', async () => { | ||
| const collectionLocators = buildCollectionLocators(page); | ||
| const hasDiscardButton = await collectionLocators.discardAllAndRemoveButton().isVisible().catch(() => false); | ||
|
|
||
| if (hasDiscardButton) { | ||
| await collectionLocators.discardAllAndRemoveButton().click( | ||
| options.forceDiscard ? { force: true } : undefined | ||
| ); | ||
| } else { | ||
| await collectionLocators.removeButton().click(); | ||
| } | ||
|
|
||
| await collectionLocators.removeModal().waitFor({ state: 'hidden', timeout: 5000 }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Click Remove before probing for the discard dialog.
Discard All and Remove is a follow-up confirmation, so checking for it before removeButton().click() means dirty collections still fall through the normal branch and never complete the discard step. This helper therefore does not actually handle the draft-confirmation flow it advertises.
Proposed fix
export const confirmRemoveCollection = async (
page: Page,
options: { forceDiscard?: boolean } = {}
) => {
await test.step('Confirm Remove Collection', async () => {
const collectionLocators = buildCollectionLocators(page);
- const hasDiscardButton = await collectionLocators.discardAllAndRemoveButton().isVisible().catch(() => false);
-
- if (hasDiscardButton) {
- await collectionLocators.discardAllAndRemoveButton().click(
- options.forceDiscard ? { force: true } : undefined
- );
- } else {
- await collectionLocators.removeButton().click();
- }
+ await collectionLocators.removeButton().click();
+
+ const discardButton = collectionLocators.discardAllAndRemoveButton();
+ const needsDiscardConfirm = await discardButton
+ .waitFor({ state: 'visible', timeout: 1000 })
+ .then(() => true)
+ .catch(() => false);
+
+ if (needsDiscardConfirm) {
+ await discardButton.click(options.forceDiscard ? { force: true } : undefined);
+ }
await collectionLocators.removeModal().waitFor({ state: 'hidden', timeout: 5000 });
});
};📝 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.
| await test.step('Confirm Remove Collection', async () => { | |
| const collectionLocators = buildCollectionLocators(page); | |
| const hasDiscardButton = await collectionLocators.discardAllAndRemoveButton().isVisible().catch(() => false); | |
| if (hasDiscardButton) { | |
| await collectionLocators.discardAllAndRemoveButton().click( | |
| options.forceDiscard ? { force: true } : undefined | |
| ); | |
| } else { | |
| await collectionLocators.removeButton().click(); | |
| } | |
| await collectionLocators.removeModal().waitFor({ state: 'hidden', timeout: 5000 }); | |
| await test.step('Confirm Remove Collection', async () => { | |
| const collectionLocators = buildCollectionLocators(page); | |
| await collectionLocators.removeButton().click(); | |
| const discardButton = collectionLocators.discardAllAndRemoveButton(); | |
| const needsDiscardConfirm = await discardButton | |
| .waitFor({ state: 'visible', timeout: 1000 }) | |
| .then(() => true) | |
| .catch(() => false); | |
| if (needsDiscardConfirm) { | |
| await discardButton.click(options.forceDiscard ? { force: true } : undefined); | |
| } | |
| await collectionLocators.removeModal().waitFor({ state: 'hidden', timeout: 5000 }); |
🤖 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/utils/page/collection.ts` around lines 48 - 60, The Confirm Remove
Collection helper is checking for discard-first instead of triggering the
initial remove action, so the draft-confirmation flow is never reached. Update
buildCollectionLocators usage in Confirm Remove Collection to click
removeButton() first, then detect and handle discardAllAndRemoveButton() if it
appears, using options.forceDiscard only for that follow-up click before waiting
on removeModal().hide.
3 participants
Description
Added testcase for open collection and refactored a file into one folder
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