-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(runner): hide APIResponse.*
calls from results
#34909
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/playwright-test/test-step.spec.ts:1451
- Changing the annotation field from an object to an array might cause compatibility issues if downstream consumers expect an object format. Please ensure all related processing is updated to support an array of annotations.
annotation: [
@@ -1462,6 +1465,11 @@ test('reading network request / response should not be listed as step', { | |||
page.on('response', async response => { | |||
await response.text(); | |||
}); | |||
await page.route('**/*', async route => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a broad route pattern ('**/*') could unintentionally intercept more network requests than intended. Consider narrowing the pattern to target only the requests relevant to the test.
await page.route('**/*', async route => { | |
await page.route('${server.EMPTY_PAGE}', async route => { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks copilot, but this is used in tests.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"7 flaky38672 passed, 793 skipped Merge workflow run. |
@@ -258,7 +258,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({ | |||
onApiCallBegin: (data: ApiCallData) => { | |||
const testInfo = currentTestInfo(); | |||
// Some special calls do not get into steps. | |||
if (!testInfo || data.apiName.includes('setTestIdAttribute') || data.apiName === 'tracing.groupEnd') | |||
if (!testInfo || data.apiName.includes('setTestIdAttribute') || data.apiName === 'tracing.groupEnd' || data.apiName.startsWith('apiResponse.')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper way is to wrap all methods of APIResponse
class with _wrapApiCall(func, true)
. That will account for tracing, steps and debug logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That only exists for ChannelOwners though, and APIResponse
isn't an interface inside protocol.yml, but only an object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but you can call it on this._request
that is a channel owner.
Closes #34840. We weren't hiding
APIResponse
calls, but they're all read-only so they shouldn't be visible in the test report.markAsInternalType
wasn't available inAPIResponse
because it's not a channel owner, so I had to add an explicit exclusion.