fix(playwright): interpolate request url with odata param#6428
fix(playwright): interpolate request url with odata param#6428bijin-bruno merged 1 commit intomainfrom
Conversation
WalkthroughUpdated test preference configuration and refactored test selectors to use testid-based locators instead of CSS nth-child selectors for improved element targeting reliability. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/interpolation/interpolate-request-url.spec.ts (1)
10-18: Consider using test.step for better report readability.As per coding guidelines, promote the use of
test.stepto make generated reports easier to read. Both test cases would benefit from steps like "Navigate to request", "Send request", and "Verify response".Example refactor for the first test:
test('Interpolate basic path params', async ({ pageWithUserData: page }) => { const locators = buildCommonLocators(page); await test.step('Navigate to echo-request-url', async () => { await locators.sidebar.collection('interpolation').click(); await locators.sidebar.request('echo-request-url').click(); }); await test.step('Send request and verify response', async () => { await sendRequest(page, 200); const responsePreview = page.getByTestId('response-preview-container').locator('.CodeMirror-scroll'); const texts = await responsePreview.allInnerTexts(); await expect(texts.some((d) => d.includes(`"url": "/path/some-data"`))).toBe(true); }); });Also applies to: 20-27
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/interpolation/init-user-data/preferences.json(1 hunks)tests/interpolation/interpolate-request-url.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
tests/interpolation/interpolate-request-url.spec.ts
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/interpolation/interpolate-request-url.spec.tstests/interpolation/init-user-data/preferences.json
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Use consistent patterns and helper utilities where they improve clarity. Prefer shared test utilities over copy-pasted setup code, but only when it actually reduces complexity
⏰ 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). (6)
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (1)
tests/interpolation/init-user-data/preferences.json (1)
2-2: Verify the window state change is intentional.Changing
maximizedfromtruetofalsemay improve test stability, but this seems unrelated to the PR objective about URL interpolation. Please confirm this change is necessary and whether it addresses a specific test issue.
| await sendRequest(page, 200); | ||
|
|
||
| const texts = await page.locator('div:nth-child(2) > .CodeMirror-scroll').allInnerTexts(); | ||
| const texts = await page.getByTestId('response-preview-container').locator('.CodeMirror-scroll').allInnerTexts(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excellent selector improvement, but extract the repeated locator.
Replacing the brittle nth-child selector with getByTestId('response-preview-container') is a solid improvement for test stability. However, this exact locator pattern is repeated on lines 16 and 25. As per coding guidelines, extract it to a locator variable.
Apply this diff:
test('Interpolate basic path params', async ({ pageWithUserData: page }) => {
const locators = buildCommonLocators(page);
await locators.sidebar.collection('interpolation').click();
await locators.sidebar.request('echo-request-url').click();
await sendRequest(page, 200);
- const texts = await page.getByTestId('response-preview-container').locator('.CodeMirror-scroll').allInnerTexts();
+ const responsePreview = page.getByTestId('response-preview-container').locator('.CodeMirror-scroll');
+ const texts = await responsePreview.allInnerTexts();
await expect(texts.some((d) => d.includes(`"url": "/path/some-data"`))).toBe(true);
});
test('Interpolate oData path params', async ({ pageWithUserData: page }) => {
const locators = buildCommonLocators(page);
await locators.sidebar.request('echo-request-odata').click();
await sendRequest(page, 200);
- const texts = await page.getByTestId('response-preview-container').locator('.CodeMirror-scroll').allInnerTexts();
+ const responsePreview = page.getByTestId('response-preview-container').locator('.CodeMirror-scroll');
+ const texts = await responsePreview.allInnerTexts();
await expect(texts.some((d) => d.includes(`"url": "/path/Category('category123')/Item(item456)/foobar/Tags(%22tag%20test%22)"`))).toBe(true);
});Also applies to: 25-25
🤖 Prompt for AI Agents
In tests/interpolation/interpolate-request-url.spec.ts around lines 16 and 25,
the locator
getByTestId('response-preview-container').locator('.CodeMirror-scroll') is
duplicated; extract it into a single const (e.g., const responsePreview =
page.getByTestId('response-preview-container').locator('.CodeMirror-scroll')) at
the top of the test or before use and replace both occurrences on lines 16 and
25 with the new variable to avoid repetition and improve maintainability.
Description
Fixes playwright test - interpolate request url with odata param
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
✏️ Tip: You can customize this high-level summary in your review settings.