Skip to content
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

feat: Display warnings only on test failure #35464

Merged
merged 18 commits into from
Apr 8, 2025
Merged

Conversation

agg23
Copy link
Contributor

@agg23 agg23 commented Apr 2, 2025

Improves warning display via terminal reporters. Warnings will only be displayed if the test failed. Warning messages for unawaited promises display a prompt for the corresponding eslint rules for more detailed investigation.

Screenshot 2025-04-07 at 8 29 28 AM Screenshot 2025-04-07 at 8 29 38 AM

This comment has been minimized.

const testRun = new TestRun(this._config, reporter);
const status = await runTasks(testRun, [
...createPluginSetupTasks(this._config),
createLoadTask('in-process', { failOnLoadErrors: true, filterOnly: false })
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... If we store warnings in .last-run.json, why do we need to load any tests? I'd think we have all the information needed for the warning in the json, so we can just print it? If so, I'd put all this logic into a separate file warnings.ts and let it manage both the "warnings data in last-run.json" and the terminal output.

That said, I think we should make warnings conveniently available to other reporters. For example, do we want to show warning per test result in the html reporter, or do we want a similar summary? If we want a summary, perhaps generate it and attach? Needs brainstorming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storing all of the necessary test information is valid, but I didn't want to deviate significantly from the current patterns. If we did store more information than just test ID, would we store code frames? Or would we just store the test name and still fetch the code frame at runtime?


You don't consider the annotation reporter API to be sufficient? A reporter can read all warning annotations onEnd.

@agg23 agg23 changed the title feat: Dedicated --show-warnings command for test runner feat: Display warnings only on test failure Apr 7, 2025
@agg23 agg23 requested a review from dgozman April 7, 2025 15:35

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Looks good!

@agg23 agg23 merged commit 8beaf3c into microsoft:main Apr 8, 2025
29 checks passed
Copy link
Contributor

github-actions bot commented Apr 8, 2025

Test results for "tests 1"

5 flaky ⚠️ [firefox-page] › page/page-evaluate.spec.ts:424:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-output.spec.ts:80:5 › should show console messages for test @macos-latest-node18-1
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @ubuntu-latest-node22-1
⚠️ [webkit-library] › library/video.spec.ts:475:5 › screencast › should scale frames down to the requested size @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18

39109 passed, 807 skipped
✔️✔️✔️

Merge workflow run.

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