Add tests for WebGLRenderLists, WebGLRenderList

[gkjohnson]

In this PR I've added tests for every function for both WebGLRenderLists and WebGLRenderList. I know these classes are considered delicate but I'm still very interested in getting Group.renderOrder fixed and something like #18599 merged so hopefully this will instill more confidence in changes made to this part of the code.

Note that as well as adding tests this change exports WebGLRenderList from WebGLRenderLists.js and makes renderItems public to make the file more testable.

@mrdoob if there's anything else we can do to make this class easier / more comfortable to update let me know!

[Mugen87]

I'm not sure we had this before (meaning making something public so it is accessible for unit tests).

@mrdoob Do we need to update the TS declaration file in this case?

[mrdoob]

I'm not sure we had this before (meaning making something public so it is accessible for unit tests).

Hmm... I feel like it's worth it.

@mrdoob Do we need to update the TS declaration file in this case?

I don't know. @gkjohnson what do you think?

[Mugen87]

I thought about this tonight and have a concern.

Unit tests should focus on testing the public interface of a component. It's not ideal if internals have to be exposed in order to write "better" unit tests. In general, unit tests should make no assumption about the implementation. Otherwise you have to refactor unit tests when internals change, even if the public interface stays the same.

Hence, I think it's better to not expose renderItems and WebGLRenderList.

[gkjohnson]

Unit tests should focus on testing the public interface of a component. It's not ideal if internals have to be exposed in order to write "better" unit tests.

Generally I agree but in this case finish can't really be tested otherwise because it has important behavior that otherwise isn't testable from the outside the class -- it's intended to remove references to objects so they can be garbage collected and I think it's valuable to make sure that's working right. If you have other suggestions on how to check that otherwise I'm happy to try another approach!

@mrdoob Do we need to update the TS declaration file in this case?

I don't know. @gkjohnson what do you think?

I think the typescript definitions files are only really useful for classes and members that we expect people to use publicly (it's really just useful for autocomplete and typescript error reporting) so I don't think we need to include it here.

[mrdoob]

How about we make these methods public temporarily?
And we add comments in the code to communicate that.

[gkjohnson]

How about we make these methods public temporarily?

Can you explain what you mean by that? "Temporarily" as in "we'll make them public and see how it goes" and maybe remove them later? Either way I'll add WebGLRenderList and renderItems to the typescript definitions and add a comment saying they were made public for the sake of testing.

[mrdoob]

We're making them public so you can get Group.renderOrder fixed. Once it's fixed we can make them private again.

[gkjohnson]

So you want to remove the tests for the finish function later, then? To me it makes sense to keep those tests to make sure that finish function doesn't silently break in the future but I'm supportive either way. I've gone ahead and added the comment added renderItems to the typescript definitions.

[gkjohnson]

Ping @mrdoob

Is there anything you want changed here? I expected this to be a relatively easy merge considering is basically just tests. If temporarily exposing render items is the problem then I can adjust that.

I'd still like to move the group render order update forward. Order dependent effects are very fragile in our application at the moment and I'm finding difficult to explain the idiosyncrasies of this part of the renderer to other users / developers on my team.

[mrdoob]

Can we tell TypeScript people that this is temporary somehow?

[gkjohnson]

I don't think so. If we don't want people to use this field because we'll plan to remove it then I don't think we should add it into the typescript file. That will make more people aware of it via autocomplete.

If we'd like to further denote it has private or unusable we could prefix with a special character and expose it as something like _renderItems?

[gkjohnson]

@mrdoob I've removed the variable from the typescript definition file. If there's still a hesitation about exposing renderItems and prefixing something like an underscore isn't an acceptable solution then maybe it's best to remove the tests for finish() to get this merged and I'll manually validate the behavior in the upcoming render order PRs.

Let me know what you'd prefer!

[Mugen87]

I vote to remove the tests for finish() so renderItems does not need to be exposed.

[gkjohnson]

Above it was mentioned that exposing this field for the tests would be worth it so I feel like I'm getting conflicting direction and I don't know what to listen to. I made all the requested changes to make it mergeable and I've offered other solutions to address the issues as I understand them but I'm not getting comments on it. At this point I feel like I'm left to keep guessing at changes to make until something is agreeable.

The aversion to making code automatically testable is odd to me. It's unclear how or whether the behavior of finish was validated in the first place because it can't be queried from the browser, either. Looking at the PR that introduced finish there was a bug that adding an automated test could have caught but was only found by carefully tracing the algorithm. In my opinion that's a problem.

It seems the easiest thing to do for now is just remove it, though. I've adjusted the PR to comment out the tests for finish and remove the public renderItems field. This way I can at least use the test in the upcoming PR.

If having commented out test is still problematic then please let me know and I'll remove it.

[mrdoob]

Thanks!