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

Fix/RTTR: find manually managed three components #1977

Conversation

DoctypeRosenthal
Copy link

@DoctypeRosenthal DoctypeRosenthal commented Jan 8, 2022

We came across a missing feature/bug(?) in test renderer where it would throw when using r3f together with manually instantiated and managed three components:
image

Feedback required:

  • was this feature missing intentionally?
  • did I overlook any cases that should be tested too?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 8, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4e4eebb:

Sandbox Source
example Configuration

@DoctypeRosenthal DoctypeRosenthal force-pushed the fix/RTTR-findAll()-with-manually-managed-three-components branch 2 times, most recently from 58a9b45 to e12511a Compare January 8, 2022 15:43
@DoctypeRosenthal DoctypeRosenthal force-pushed the fix/RTTR-findAll()-with-manually-managed-three-components branch from e12511a to 90f834d Compare January 8, 2022 15:45
@DoctypeRosenthal DoctypeRosenthal marked this pull request as draft January 8, 2022 15:46
@DoctypeRosenthal DoctypeRosenthal marked this pull request as ready for review January 8, 2022 15:47

it('should also find three components which are not mounted via r3f and not throw', async () => {
const { scene } = await ReactThreeTestRenderer.create(<WithManuallyManagedComponent />)
expect(() => scene.findByProps({ name: MANUALLY_MOUNTED_COMPONENT_NAME })).not.toThrow()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important because the test below just tests a specific error but not if no error is thrown at all.

@DoctypeRosenthal DoctypeRosenthal changed the title Fix/RTTR: findAll() with manually managed three components Fix/RTTR: find manually managed three components Jan 8, 2022
return this._fiber.__r3f.memoizedProps
return this._fiber.__r3f?.memoizedProps ?? this._fiber
Copy link
Member

@CodyJasonBennett CodyJasonBennett Jan 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'd do the same for the parent getter below. External parts of the threejs scene don't have __r3f appended to them. Helpers are one case of this but also <primitive /> or results of loaders.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be returning the node if memoized props is not there, you're confusing props with the instance properties.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you suggest then @joshuaellis ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well you're not passing props to the object cause it's not rendered by react. So it should either return an empty object or not be accessible in the tree which would be even harder to do. I don't mind the former.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you cannot use the .findByProps(), .findAll() etc. methods at all anymore because they throw.

This isn't necessarily true:

expect(() => scene.find((node) => node.instance.name === MANUALLY_MOUNTED_COMPONENT_NAME)).not.toThrow()

will not throw currently.

as you said, in this particular scenario the return-value of .props is equal to .instance for "manually" mounted components.

Which makes the public facing API confusing. To someone new to an existing codebase with implemented tests, they might start to ask "why can I access some props but not all of them on certain components". It should act differently for two different nodes.

I'm not opposed to extending the API, we could have a new methods called findByInstanceProperties which could look like this:

  public findByInstanceProperties = (properties: Obj): ReactThreeTestInstance =>
    expectOne(this.findAllByInstanceProperties(properties), `with instance properties: ${JSON.stringify(properties)}`)

  public findAllByInstanceProperties = (properties: Obj): ReactThreeTestInstance[] =>
    findAll(this, (node: ReactThreeTestInstance) => Boolean(node.instance && matchProps(node.instance, properties)))

This would create a clear mental differentiation between the two APIs whilst including the feature you're requesting (which I have no problem with adding, it's just about how we add it).

What do you think? Also for more opinions cc @CodyJasonBennett

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument you made regarding the inconsistent API is true: returning a subset when the component is mounted via r3f and returning all props when it isn't is not good design. Your idea of adding a .findByInstanceProps() method is great!
But then you'd write .findByProps({name: "BOX-A"}) when you only have a "BOX-A" which is mounted via r3f and .findByInstanceProps({name: "BOX-B"}) when you have "BOX-B" which isn't mounted via r3f. This doesn't seem quite elegant from the client-code perspective to me because you'd have to use different testing methods based on implementation details, which you actually want to ignore in black box tests that focus on user interaction. What do you say? Maybe there is a third even better way?

Copy link
Author

@DoctypeRosenthal DoctypeRosenthal Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides: I just tested your approach with the .find() method and it throws too because it uses .getChildren() which acesses fiber.__r3f.objects.

Cannot read properties of undefined (reading 'objects')

Copy link
Author

@DoctypeRosenthal DoctypeRosenthal Jan 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that .findByProps() is actually not User oriented as it is recommended in react-testing-library since it tests for things which the user cannot see (e.g. the name prop). It is comparable to .getByTestId() which is not recommended, not to .getByText() e.g. in react-testing-lib.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to discuss this in an issue. There's a clear separation between react and three, and I don't want design to hold back fixes if we can avoid it.

@DoctypeRosenthal
Copy link
Author

DoctypeRosenthal commented Jan 8, 2022

Can you help me with fixing the CI testing error @CodyJasonBennett?

@CodyJasonBennett
Copy link
Member

Looking into it. Strange that this will pass locally.

@DoctypeRosenthal
Copy link
Author

DoctypeRosenthal commented Jan 8, 2022

I was surprised by that either...

@joshuaellis joshuaellis linked an issue Jan 9, 2022 that may be closed by this pull request
@CodyJasonBennett
Copy link
Member

I think we'll want to revisit this with a query API since there is a meaningful difference between props known to JSX and the underlying three.js instance, but the semantics of RTTR are wrong here to begin with. I'll look to resolve this with #2338, since props should be clearly communicated as a narrower search.

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.

Test renderer throws when using components not mounted via r3f
3 participants