-
-
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
Add tests for WebGLRenderLists, WebGLRenderList #19346
Conversation
@@ -159,6 +159,7 @@ function WebGLRenderList() { | |||
} | |||
|
|||
return { | |||
renderItems: renderItems, |
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'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?
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'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?
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 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
.
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.
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.
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.
How about we make these methods public temporarily?
And we add comments in the code to communicate that.
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.
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.
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.
We're making them public so you can get Group.renderOrder
fixed. Once it's fixed we can make them private again.
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.
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.
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. |
@@ -22,6 +22,7 @@ export interface RenderItem { | |||
|
|||
export class WebGLRenderList { | |||
|
|||
renderItems: Array<RenderItem>; |
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.
Can we tell TypeScript people that this is temporary somehow?
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 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
?
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.
@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!
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 vote to remove the tests for finish()
so renderItems
does not need to be exposed.
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.
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.
Thanks! |
In this PR I've added tests for every function for both
WebGLRenderLists
andWebGLRenderList
. I know these classes are considered delicate but I'm still very interested in gettingGroup.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
fromWebGLRenderLists.js
and makesrenderItems
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!