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(json-reporter): added parallelIndex to TestResult #34740

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cTangonan123
Copy link

@cTangonan123 cTangonan123 commented Feb 12, 2025

added parallelIndex to JSONReportTestResult and serializeTestResult

Fixes #33036

Co-authored-by: Neel Deshmukh <neel.deshmukh1@gmail.com>
Co-authored-by: Marcelo Villalobos Diaz <mvillalobosdiaz@csumb.edu>

This comment has been minimized.

@cTangonan123
Copy link
Author

@microsoft-github-policy-service agree

@mxschmitt mxschmitt changed the title feat(parallelIndex): added parallelIndex to JSONReportTestResult (#33036) feat(json-reporter): added parallelIndex to JSONReportTestResult (#33036) Feb 12, 2025
@mxschmitt mxschmitt changed the title feat(json-reporter): added parallelIndex to JSONReportTestResult (#33036) feat(json-reporter): added parallelIndex to TestResult (#33036) Feb 12, 2025
@@ -291,6 +291,7 @@ export interface JSONReportError {

export interface JSONReportTestResult {
workerIndex: number;
parallelIndex: number;
Copy link
Member

Choose a reason for hiding this comment

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

this file is actually generated by this file - so you need to add it there as well.

Copy link
Author

Choose a reason for hiding this comment

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

Oh we missed that, I'll be sure to relay with my co-authors and get those changes made.

Copy link
Member

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

We require a new test for every new functionality which gets added. Do you mind adding one here?

@mxschmitt mxschmitt changed the title feat(json-reporter): added parallelIndex to TestResult (#33036) feat(json-reporter): added parallelIndex to TestResult Feb 12, 2025
@cTangonan123
Copy link
Author

We require a new test for every new functionality which gets added. Do you mind adding one here?

for the test, would you prefer to have multiple tests ran in parallel or just that parallelIndex is accessible?

@mxschmitt
Copy link
Member

Ideally one test which runs e.g. 3 tests (pass, fail, pass) inside with 1 worker. The json report you'll assert then that the parallelIndex and workerIndex look correct.

Why pass/fail/pass? Because we restart the worker if it fails, so test 3 gets a different workerIndex compared to test 2.

@cTangonan123
Copy link
Author

cTangonan123 commented Feb 12, 2025

Ideally one test which runs e.g. 3 tests (pass, fail, pass) inside with 1 worker. The json report you'll assert then that the parallelIndex and workerIndex look correct.

Why pass/fail/pass? Because we restart the worker if it fails, so test 3 gets a different workerIndex compared to test 2.

Thank you for such a concise example and explanation as it aids us in better understanding the distinction between workerIndex and parallelIndex.

So to my understanding the expected behavior for the above example could look something like this?

Test parallelIndex workerIndex pass/fail
1 0 1 pass
2 0 1 fail
3 0 2 pass

This comment has been minimized.

@mxschmitt
Copy link
Member

mxschmitt commented Feb 13, 2025

Yes. Maybe it would be neat to say workers: 2 (with more tests if needed) so we also have a parallelIndex which is non zero.

@cTangonan123
Copy link
Author

Yes. Maybe it would be neat to say workers: 2 (with more tests if needed) so we also have a parallelIndex which is non zero.

Thanks so much for the extra input! My co-authors will be meeting tonight and over the weekend, I think we can approach the first example test(and get it working) then, look towards modifying the test to handle 2 workers.

Copy link
Contributor

Test results for "tests 1"

12 flaky ⚠️ [chromium-library] › tests/library/browsertype-connect.spec.ts:400:5 › run-server › should terminate network waiters @chromium-ubuntu-22.04-node22
⚠️ [firefox-library] › tests/library/role-utils.spec.ts:442:5 › svg role=presentation @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:125:3 › should authenticate @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:109:1 › should show tracing.group in the action list with location @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/video.spec.ts:358:5 › screencast › should expose video path blank popup @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-filechooser.spec.ts:24:5 › should upload multiple large files @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38542 passed, 793 skipped
✔️✔️✔️

Merge workflow run.

cTangonan123 and others added 2 commits February 14, 2025 16:45
…ts and added initial test to reporter-json.spec.ts

Co-authored-by: Neel Deshmukh <neel.deshmukh1@gmail.com>
Co-authored-by: Marcelo Villalobos Diaz <mvillalobosdiaz@csumb.edu>
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.

[Feature] Include parallelIndex in the JSON report
3 participants