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

chore(tests): add simple tests for ref rerender and subscription #88

Merged
merged 2 commits into from Feb 20, 2021

Conversation

YPAzevedo
Copy link
Contributor

@YPAzevedo YPAzevedo commented Feb 19, 2021

Adds some simple tests for ref that was introduced in #62 and #65 fixes #66

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 19, 2021

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 086283a:

Sandbox Source
React Configuration
React Typescript Configuration

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Thanks for your work!
overall it looks good.
the tests cover #62.
they don't cover #65. (it's tricky, probably needs some discussions.)

await findByText('count: 0')

fireEvent.click(getByText('button'))
await findByText('count: 0')
Copy link
Member

Choose a reason for hiding this comment

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

I guess this check is weak. It doesn't guarantee that the count keeps 0, does it?

Copy link
Member

Choose a reason for hiding this comment

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

can you double check if this fails if you remove ref? I might be wrong though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it does still pass without ref 🤔 what would be your recommendation here to make the test stronger?

Copy link
Contributor Author

@YPAzevedo YPAzevedo Feb 19, 2021

Choose a reason for hiding this comment

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

added the wait for the Promise.resolve to make the assertion of findByText wait for the changes to be flushed. now it fails with no ref 😀

no ref - count: 0 - count: 0 -> FAIL
no ref - count: 0 - count: 1 -> PASS
ref - count: 0 - count: 0 -> PASS
ref - count: 0 - count: 1 -> FAIL

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wasn't so sure about the best way.
Promise.resolve() is a nice hack, even though it depends on some implementation details. We are doing it in other tests, anyway.
Good to go. And hope someone has a better idea in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thanks! i'll see if theres a better solution and update this and the other tests! 👍

obj.nested.count += 1

await Promise.resolve()
expect(handler).toBeCalledTimes(0)
Copy link
Member

Choose a reason for hiding this comment

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

this is probably ok.

await findByText('count: 0')

fireEvent.click(getByText('button'))
await findByText('count: 0')
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wasn't so sure about the best way.
Promise.resolve() is a nice hack, even though it depends on some implementation details. We are doing it in other tests, anyway.
Good to go. And hope someone has a better idea in the future.

@dai-shi dai-shi merged commit 3ed0dac into pmndrs:master Feb 20, 2021
@dai-shi dai-shi mentioned this pull request Feb 20, 2021
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.

Add test for ref
2 participants