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

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

Merged
merged 2 commits into from
Feb 20, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af54a6f
chore(tests): add simple tests for ref rerender and subscription
YPAzevedo Feb 19, 2021
086283a
chore(tests): wait to assert no changes were made
YPAzevedo Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion tests/basic.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { StrictMode, useRef, useEffect, useState } from 'react'
import { fireEvent, render, waitFor } from '@testing-library/react'
import { proxy, useProxy } from '../src/index'
import { proxy, ref, useProxy } from '../src/index'

it('simple counter', async () => {
const obj = proxy({ count: 0 })
Expand Down Expand Up @@ -344,3 +344,29 @@ it('render from outside', async () => {
fireEvent.click(getByText('toggle'))
await findByText('count: 1')
})

it('should not trigger re-render when mutating object wrapped in ref', async () => {
const obj = proxy({ nested: ref({ count: 0 }) })

const Counter: React.FC = () => {
const snapshot = useProxy(obj)
return (
<>
<div>count: {snapshot.nested.count}</div>
<button onClick={() => ++obj.nested.count}>button</button>
</>
)
}

const { getByText, findByText } = render(
<StrictMode>
<Counter />
</StrictMode>
)

await findByText('count: 0')

fireEvent.click(getByText('button'))
await Promise.resolve()
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 •
edited

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! 👍

})
14 changes: 13 additions & 1 deletion tests/subscribe.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { proxy, subscribe } from '../src/index'
import { proxy, ref, subscribe } from '../src/index'

describe('subscribe', () => {
it('should call subscription', async () => {
Expand Down Expand Up @@ -83,4 +83,16 @@ describe('subscribe', () => {
await Promise.resolve()
expect(handler).toBeCalledTimes(1)
})

it('should not call subscription for objects wrapped in ref', async () => {
const obj = proxy({ nested: ref({ count: 0 }) })
const handler = jest.fn()

subscribe(obj, handler)

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.

})
})