Please add back unit tests for addons

[catalin-enache]

Description

I have this PR: https://github.com/mrdoob/three.js/pull/28441 fixing a regression for BufferGeometryUtils.mergeVertices.

I had an older PR https://github.com/mrdoob/three.js/pull/28397 (merged) fixing FBXLoader morph attributes.

For the BufferGeometryUtils.mergeVertices I'm sure it's a regression (I found the commit).

For FBXLoader I didn't checked but I believe it was a regression not introduced by changes to FBXLoader itself but by the ShapeUtils.triangulateShape not returning triangles anymore in some conditions and FBXLoader relied on ShapeUtils.triangulateShape to always return triangles (a regression scenario if not an actual regression).

For BufferGeometryUtils.mergeVertices PR I also created an unit test, in order to prevent a regression on that specific functionality in the future.

@Mugen87 approved the reintroducing of unit tests for addons (providing some hints too). @donmccurdy also likes the idea but suggested to create this particular issue where we could discuss why we need them and prevent an eventual re-removal of unit tests for addons.

IMO: Why we need them.

Long story short: to prevent regression (obviously).

Why we would care about preventing regression for addons.

As a ThreeJs user I cannot live without addons. As I said in the other commit, I see ThreeJS as a framework with all "batteries included". What would ThreeJS be without FBX/GLTF loader, without OrbitControls and all that awesome stuff in addons ? Like a car made of the engine only (no wheels, not anything else). That would be a theoretical car, but not very useful.

Probably ThreeJs could still be used in generating shapes for scientific purposes, but one could not create a game without good asset loaders.

For me FBX and GLTF loaders are the keys to the castle since the big majority of assets are delivered in these formats (with FBX still being the most used AFAIK) If they are not reliable the entire library gets shadowed by doubt for me as a ThreeJS user.

Other super useful things in addons are non standard image loaders (EXRLoader to mention just one of them).

Assest and image loaders (which most of them unfortunately live in addons) are crucial for being able to make a game using the library. IMO they should be treated as first citizens and for that, they need to be supported with unit-tests.

But guys, everything in addons is awesome, and helpful when needed. If buggy, then the work spent in making them does not add value to the library but on the contrary, raise general questions about the library reliability overall.

So I ask for getting back the unit tests for the addons, and start to throw something there, at least for new changes, so that they can be protected and remain reliable.

Another thing that would be helpful in testing (in particular the loaders) would be having the possibility to load an asset from disk, to see if the loader is parsing it correctly, using some kind of mock server. In that regard I'll probably make another feature request along with a PR in the future (I'd like to throw a test into FBXLoader for that specific fix related to morphArttributes length).

That would be my opinion regarding why we need unit tests for addons (at least for the critical ones).

It should not be a burden, the coverage should not be enforced. Whoever wants to make a test should be able to make one (no doubt, it will help with regression).

Solution

not needed

Alternatives

not needed

Additional context

No response

[Mugen87]

Originally the unit tests for examples/jsm have been removed to have less things to maintain in the repository. At that time, I have also argued to improve the test coverage in the core first before thinking about the examples.

I admit that with increasing importance of examples/jsm (or addons) this assessment isn't valid anymore. So I'm okay with adding unit tests again for addons. The necessary npm scripts and file conventions are explained in https://github.com/mrdoob/three.js/pull/28441#discussion_r1606541088 (the setup is fortunately easy).

[RemusMar]

I think the unit tests for addons are useful. On the other hand, I think the GLTF loader should be added to the core.

[mrdoob]

Something I would like to do different this time is to avoid stubs: https://github.com/mrdoob/three.js/pull/25398

Lets make sure we have real tests and not placeholders.

[mrdoob]

@RemusMar

On the other hand, I think the GLTF loader should be added to the core.

Are you sure about that? GLTF is planning on adding physics and interaction... The core would go from 170kb to 3MB... 😬

[RemusMar]

@mrdoob

Are you sure about that? GLTF is planning on adding physics and interaction... The core would go from 170kb to 3MB... 😬

Ricardo, I'm sure about that, but I'm not talking about physics (we can do that later "per request"). The current GLTF Loader minified is around 45KB only !!! It would be a great addition. Take a look at Babylon.js (over 5MB) and PlayCanvas.js (over 1MB). We don't live in the dial-up era anymore ...

[donmccurdy]

Something I would like to do different this time is to avoid stubs: https://github.com/mrdoob/three.js/pull/25398

Lets make sure we have real tests and not placeholders.

I'm comfortable with this, thanks. I do think unit tests — particularly for BufferGeometryUtils and exporters — would make it easier to review PRs and to avoid regressions.

I'm not ready to have an opinion on additions to core at the moment. :)

[Mugen87]

A basic setup for unit tests for addons is now in place via #28441. Let's see how it goes^^.