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

Puppeteer E2E test: Multi-page in-browser parallelism #25386

Merged
merged 33 commits into from Feb 6, 2023

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Jan 31, 2023

Related issue: #24109

Description

Add multi-page parallelism to Puppeteer and also decrease CI parallelism to 4 threads.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Jan 31, 2023

Strange that tests fail... Seems like a page is used simultaneously in two attempts (but that should be impossible because of the lock)?..

 file:///home/runner/work/three.js/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/common/ExecutionContext.js:298
        throw new Error('Execution context was destroyed, most likely because of a navigation.');
              ^

Error: Execution context was destroyed, most likely because of a navigation.
    at rewriteError (file:///home/runner/work/three.js/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/common/ExecutionContext.js:298:15)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async ExecutionContext._ExecutionContext_evaluate (file:///home/runner/work/three.js/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/common/ExecutionContext.js:241:56)
    at async ExecutionContext.evaluate (file:///home/runner/work/three.js/three.js/node_modules/puppeteer-core/lib/esm/puppeteer/common/ExecutionContext.js:122:16)
    at async Promise.all (index 0)
    at async file:///home/runner/work/three.js/three.js/test/e2e/puppeteer.js:272:16
Error: Process completed with exit code 1.

@LeviPesin LeviPesin marked this pull request as draft January 31, 2023 07:00
@LeviPesin
Copy link
Contributor Author

LeviPesin commented Jan 31, 2023

Especially strange that I don't see that problem when running locally...
UPD: Seeing it now...

@LeviPesin
Copy link
Contributor Author

Seems like the Mac problems are related to puppeteer/puppeteer#8693... I will investigate more.

LeviPesin added a commit to LeviPesin/three.js-1 that referenced this pull request Jan 31, 2023
mrdoob pushed a commit that referenced this pull request Jan 31, 2023
* Puppeteer: Bug fix

* Disable MacOS until a proper solution in #25386
@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 1, 2023

Headful:
First run - 0 has not failed, 1 failed on webgl_materials_video, 2 failed on webgl_multiple_elements_text and webxr_vr_video.
Second run - 0 has not failed, 1 has not failed, 2 failed on webgl_multiple_elements_text and webxr_vr_video.
Third run - 0 failed on webgl2_multisampled_renderbuffers and webgl_buffergeometry_lines, 1 has not failed, 2 failed on webgl_multiple_elements_text, webgpu_nodes_playground, webxr_ar_dragging, and webxr_vr_video.
I think these about-the-same results (and also that fails were NOT due to TimeoutError) mean that the main problem disappears when usnig headful (same as described in puppeteer/puppeteer#8693).
But it also could be that just parallelism somehow stops working in headful...

test/e2e/puppeteer.js Fixed Show fixed Hide fixed
test/e2e/puppeteer.js Fixed Show fixed Hide fixed
test/e2e/puppeteer.js Fixed Show fixed Hide fixed
@LeviPesin
Copy link
Contributor Author

Hm... Even with multiple browsers instead of multiple pages TimeoutError happens. So maybe it is not puppeteer/puppeteer#8693...

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 1, 2023

It could be fixed just by increasing networkTimeout 🤦‍♂️ I think it works now. (I will re-run the tests a few times more just to check)

@LeviPesin
Copy link
Contributor Author

Seems that it works!

@LeviPesin
Copy link
Contributor Author

Seems like sometimes Mac fails even with a large timeout...

@LeviPesin LeviPesin marked this pull request as draft February 3, 2023 07:14
@LeviPesin LeviPesin marked this pull request as ready for review February 3, 2023 07:14
@LeviPesin LeviPesin marked this pull request as draft February 3, 2023 07:28
@LeviPesin LeviPesin marked this pull request as ready for review February 3, 2023 13:17
@LeviPesin
Copy link
Contributor Author

@mrdoob I think we can merge this PR and possibly tweak the number of CI threads, browser pages, and network timeout later. (It should work currently)

@LeviPesin
Copy link
Contributor Author

Will run tests a few times to see if some examples don't work under new threshold...

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 6, 2023

@mrdoob Seems to work!

@mrdoob mrdoob merged commit 49eccbb into mrdoob:dev Feb 6, 2023
@mrdoob mrdoob added this to the r150 milestone Feb 6, 2023
@LeviPesin LeviPesin deleted the puppeteer-multi-page-parallelism branch February 6, 2023 10:44
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.

None yet

3 participants