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.

Sign up for GitHub

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

Jump to bottom

Fix/RTTR: find manually managed three components #1977

Closed
wants to merge 4 commits into from
Closed

Fix/RTTR: find manually managed three components #1977

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a90afaa
test: add test for finding manually managed components
DoctypeRosenthal Jan 8, 2022
90f834d
fix: react three test renderer working also with manually managed com…
DoctypeRosenthal Jan 8, 2022
fc5f349
test: get parent of manually managed three component without throwing
DoctypeRosenthal Jan 8, 2022
4e4eebb
fix: getting a parent of manually managed three component threw an er…
DoctypeRosenthal Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions packages/test-renderer/src/__tests__/RTTR.methods.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { BoxHelper, Object3D } from 'three'

jest.mock('scheduler', () => require('scheduler/unstable_mock'))

import * as React from 'react'

import ReactThreeTestRenderer from '../index'
import { useThree } from '@react-three/fiber/src'
import { useRef } from 'react'

describe('ReactThreeTestRenderer instance methods', () => {
const ExampleComponent = () => {
Expand All @@ -20,6 +24,28 @@ describe('ReactThreeTestRenderer instance methods', () => {
)
}

const MANUALLY_MOUNTED_COMPONENT_NAME = 'not-mounted-by-r3f'

const WithManuallyManagedComponent = () => {
const { scene } = useThree((three) => three)
const ref = useRef<Object3D>()

React.useEffect(() => {
if (!ref.current) return
const manuallyManagedComponent = new BoxHelper(ref.current)
manuallyManagedComponent.name = MANUALLY_MOUNTED_COMPONENT_NAME
manuallyManagedComponent.visible = false
scene.add(manuallyManagedComponent)
}, [])

return (
<mesh name="mesh_01" ref={ref}>
<boxBufferGeometry args={[2, 2]} />
<meshStandardMaterial color={0x0000ff} />
</mesh>
)
}

it('should pass the parent', async () => {
const { scene } = await ReactThreeTestRenderer.create(<ExampleComponent />)

Expand Down Expand Up @@ -98,4 +124,14 @@ describe('ReactThreeTestRenderer instance methods', () => {

expect(() => scene.findByProps({ color: 0x0000ff })).toThrow()
})

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.

expect(() => scene.findByProps({ name: MANUALLY_MOUNTED_COMPONENT_NAME })).not.toThrow(
"Cannot read properties of undefined (reading 'memoizedProps')",
)
const instance = scene.findByProps({ name: MANUALLY_MOUNTED_COMPONENT_NAME }).instance
expect(instance).toBeDefined()
})
})
6 changes: 3 additions & 3 deletions packages/test-renderer/src/createTestInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ export class ReactThreeTestInstance<TInstance extends Object3D = Object3D> {
}

public get instance(): Object3D {
return (this._fiber as unknown) as TInstance
return this._fiber as unknown as TInstance
}

public get type(): string {
return this._fiber.type
}

public get props(): Obj {
return this._fiber.__r3f.memoizedProps
return this._fiber.__r3f?.memoizedProps ?? this._fiber
Copy link
Member

@CodyJasonBennett CodyJasonBennett Jan 8, 2022 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

}

public get parent(): ReactThreeTestInstance | null {
Expand Down Expand Up @@ -50,7 +50,7 @@ export class ReactThreeTestInstance<TInstance extends Object3D = Object3D> {
*/
return [
...(fiber.children || []).map((fib) => wrapFiber(fib as MockInstance)),
...fiber.__r3f.objects.map((fib) => wrapFiber(fib as MockInstance)),
...(fiber.__r3f?.objects ?? []).map((fib) => wrapFiber(fib as MockInstance)),
]
} else {
return (fiber.children || []).map((fib) => wrapFiber(fib as MockInstance))
Expand Down