Skip to content

Conversation

@Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Nov 27, 2025

No description provided.

@Skn0tt Skn0tt requested a review from dgozman November 27, 2025 10:29
@Skn0tt Skn0tt self-assigned this Nov 27, 2025
@Skn0tt
Copy link
Member Author

Skn0tt commented Nov 27, 2025

gotta figure out what to do about result.errors 🤔

@Skn0tt Skn0tt marked this pull request as draft November 27, 2025 10:53
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Skn0tt Skn0tt marked this pull request as ready for review November 27, 2025 11:23
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Test results for "MCP"

2 failed
❌ [webkit] › mcp/generator.spec.ts:62 › generator_setup_page @mcp-ubuntu-latest
❌ [webkit] › mcp/video.spec.ts:63 › should work with recordVideo (isolated) @mcp-ubuntu-latest

2608 passed, 116 skipped


Merge workflow run.

@github-actions
Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [playwright-test] › reporter-html.spec.ts:362 › merged › should use different path if attachments base url option is provided @macos-latest-node18-2
❌ [playwright-test] › reporter.spec.ts:251 › merged › should not have internal error when steps are finished after timeout @macos-latest-node18-2
❌ [default-reuse] › debug-tests.spec.ts:389 › should debug multiple tests and stop on first failure @vscode-extension

10 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-event-request.spec.ts:182 › should return response body when Cross-Origin-Opener-Policy is set `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/workers.spec.ts:163 › should clear upon cross-process navigation `@firefox-ubuntu-22.04-node18`
⚠️ [playwright-test] › ui-mode-test-attachments.spec.ts:102 › should linkify string attachments `@ubuntu-latest-node18-2`
⚠️ [webkit-library] › library/browsercontext-cookies-third-party.spec.ts:319 › add 'Partitioned;' cookie via API `@webkit-ubuntu-22.04-node18`
⚠️ [webkit-library] › library/browsercontext-reuse.spec.ts:195 › reuse connect › should not cache resources `@webkit-ubuntu-22.04-node18`
⚠️ [webkit-library] › library/har.spec.ts:137 › should include postData `@webkit-ubuntu-22.04-node18`
⚠️ [webkit-library] › library/proxy.spec.ts:93 › should proxy local network requests › with other bypasses › link-local `@webkit-ubuntu-22.04-node18`
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:146 › should upload large file `@webkit-ubuntu-22.04-node18`
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:299 › should detect mime type `@webkit-ubuntu-22.04-node18`

40326 passed, 789 skipped


Merge workflow run.


_emitErrors() {
const errors = this.errors.slice(this._reportedError);
this._reportedError = this.errors.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this._reportedError = this.errors.length;
this._reportedError = Math.max(this._reportedError, this.errors.length);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just being defensive, or is there a race here that I don't see?

return;
const { result } = data;
for (const error of params.errors)
addLocationAndSnippetToError(this._config.config, error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't internal reporter do this already, upon reporter.onTestEnd? I am afraid we'll get double snippets. Perhaps this should be handled specifically in onTestPaused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved all result/step snippet adding into dispatcher, that's the cleanest way of doing things I think.

}

onStepBegin(test: TestCase, result: TestResult, step: TestStep) {
this._addSnippetToTestErrors(test, result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this will definitely produce double snippets. We should figure out this part.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "produce double snippets", you mean that we wastefully compute them twice, right? Because I don't see how we store them multiple times

expect(result.exitCode).toBe(0);
expect(result.passed).toBe(1);
}, { debug: true }, { PLAYWRIGHT_FORCE_TTY: 'true' });
await testProcess.waitForOutput('Paused at End');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need some way to resume, in case I'd like to debug fixture teardown or global teardown.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The future will show!

@Skn0tt Skn0tt requested a review from dgozman November 27, 2025 13:54
@Skn0tt Skn0tt mentioned this pull request Nov 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants