-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
#28572 Run unit tests in browser with puppeteer & express #28573
base: dev
Are you sure you want to change the base?
#28572 Run unit tests in browser with puppeteer & express #28573
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
<head> | ||
<meta charset="utf-8"> | ||
<title>ThreeJS Unit Tests - Using Files in /src</title> | ||
<link rel="stylesheet" href="../../node_modules/qunit/qunit/qunit.css"> | ||
<!-- if we don't set favicon here we have to intercept the request for it in puppeteer page to prevent console error --> | ||
<link rel="icon" type="image/x-icon" href="/files/favicon.ico" /> |
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.
Every time I run the tests I saw a console error : The resource could not be loaded , server returned 404
. It was due to this favicon. Initially I intercepted it in the new puppeteer.unit.js
and returned /files/favicon.ico for that specific request, then I realized it is less trouble to add it here.
@@ -19,6 +21,14 @@ | |||
} | |||
} | |||
</script> | |||
<script> | |||
window.QUnit.on( 'runEnd', ( runEnd ) => { |
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.
I tried to execute this in the puppeteer script inside a separate page.evaluate
but for some reason it did not run. I didn't investigate deeper since here it executes fine.
I realise that this PR is not an urgent matter but it will start adding value on the testing side right away if accepted. In the context of previous feedbacks from you guys (for other PRs), my intuition is that @donmccurdy would like the idea proposed here since it would enable (in particular) testing In the eventuality that it might have been overlooked that the issue has a PR attached, I'm also adding @Mugen87 and @mrdoob for visibility. If other things need prioritization for getting reviewed I perfectly understand. |
To be transparent, my ideal preference would be to use glTF Transform (20kb) as a dev dependency and to dynamically generate test assets for GLTFLoader, and to validate output of GLTFExporter. This makes it easier to check special cases like interleaved vertex buffers, or two meshes sharing attributes, where GLTFExporter is not (and probably should not be) designed to create output in unusual and different data layouts, and to not add more models to the repository. But I realize that solution is highly specific to glTF, so I have no objection to setting up a local server like Express for FBXLoader and other loaders. I'll defer to others on that decision. I would be curious whether a test runner like Vitest has these features built in? |
Thanks for feedback Just checked now to see if Vitest can run in real DOM and it seems it does not (vitest-dev/vitest#586) unless willing to use other libs like (mocha-vite-puppeteer) as commented in that thread. In any case switching to another unit test library is beyond the scope of this PR. My only intention is to enable running test in DOM environment (to allow testing As you mentioned, indeed, the glTF Transform solution is strictly specific to GLTF. As it can be seen this PR did succeed in running all existing tests but this time they were run in DOM env. |
I'm anticipating that the lack of response on the PR so far is due to uncertainty about the added complexity of these changes in the test framework. If that's the concern, we may need simpler alternatives. Vitest has since added an experimental browser mode, though I agree replacing a test framework would fall well outside the scope of your PR.
|
Related issue: #28572
Description
Configured unit tests to run in headless browser (launched by
puppeteer
) and usedexpress
to support assets loading.Updated README for unit tests.
Added
express
as an explicit dev dependency (it was already used in existing puppeteer script without being declared inpackage.json
- ESLint warned about that).Added
cross-env
for setting ENV variables cross platform.Updated test jobs to use the new
puppeteer.unit.js
script.Added to new jobs (sufifxed with
'headful'
) for convenience when testing locally.Created
puppeteer.unit.js
script, getting inspired from existing puppeteer script used ine2e
tests. It accepts 2 ENV parameters:TEST_PAGE
(for reusing it with core as well as addons tests) andVISIBLE
which controls the test mode ('headless'
/'headful'
)The tests results (final log) have the same format as before.
The
webonly
tests that were excluded before are now included.Consoles are intercepted and printed in CLI.
Added a new test for
FBXLoader
(which I couldn't test before due to not being able to loadfbx
asset from disk).I wanted to keep the changes in this PR focused on the configuration, and not clutter with other tests.
If this PR gets accepted I plan to add (soon) another FBXLoader test (about being able to use non native textures).