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

#28572 Run unit tests in browser with puppeteer & express #28573

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

catalin-enache
Copy link
Contributor

@catalin-enache catalin-enache commented Jun 6, 2024

Related issue: #28572

Description

Configured unit tests to run in headless browser (launched by puppeteer) and used express 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 in package.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 in e2e tests. It accepts 2 ENV parameters: TEST_PAGE (for reusing it with core as well as addons tests) and VISIBLE 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 load fbx 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).

Copy link

github-actions bot commented Jun 6, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
682.2 kB (169 kB) 682.2 kB (169 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
459.4 kB (110.9 kB) 459.4 kB (110.9 kB) +0 B

<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" />
Copy link
Contributor Author

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 ) => {
Copy link
Contributor Author

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.

@catalin-enache
Copy link
Contributor Author

catalin-enache commented Jun 12, 2024

I realise that this PR is not an urgent matter but it will start adding value on the testing side right away if accepted.
I'd very much appreciate anyone's feedback on this.

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 GLTFLoader against actual assets and (in general) testing anything inside real DOM environment, removing the blocker for some TODOs which could not be implemented before (e.g. ImageLoader#load).

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.
Sorry in advance if that was not the case and you already knew about it, in which case sorry for being a bit pushy.

If other things need prioritization for getting reviewed I perfectly understand.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 21, 2024

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?

@catalin-enache
Copy link
Contributor Author

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 ImageLoader.load and any other loader that rely on DOM to work) also providing a way to serve assets so that any loader could be fully tested, doing this with libs already used in the project so that no change is required for existing tests.

As you mentioned, indeed, the glTF Transform solution is strictly specific to GLTF.
This proposal would enable any loader to be fully tested.

As it can be seen this PR did succeed in running all existing tests but this time they were run in DOM env.
Also the new FBXLoader test requiring asset from disk passed.
No regression, just making possible new kind of tests which would contribute to stability.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 21, 2024

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.

EDIT: Sorry for the accidental close/reopen, I hit a wrong button.

@donmccurdy donmccurdy closed this Jun 21, 2024
@donmccurdy donmccurdy reopened this Jun 21, 2024
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

2 participants