Skip to content

Conversation

@achou11
Copy link
Member

@achou11 achou11 commented Feb 24, 2025

Non-exhaustive first attempt to introduce some more e2e-like tests for hooks related to projects. The approach is more data-oriented as opposed to the recommended approach of being DOM-oriented, mostly becaues it felt easier to focus on the tests I was interested in. I could imagine that we'd want to add tests that also work with a DOM to test out how the hooks interact with things like suspense and error boundaries, but think that can be a follow-up if desired.

Would've liked the tests to have more assertions to guarantee the lifecycle of hooks (e.g. checking pending status for writes or isRefetching for reads), but I'm pretty sure I'm running into the issue highlighted in TanStack/query#4379. There isn't an easy way to inject an artificial delay for every mutation and query function that interfaces with core, which I can confirm solves the issue, but open to ideas on alternative approaches (maybe setting up a worker that has some ipc overhead??).

@achou11 achou11 force-pushed the ac/projects-hooks-tests branch 2 times, most recently from 867cbd4 to 3c382cc Compare March 3, 2025 15:54
@achou11 achou11 force-pushed the ac/projects-hooks-tests branch from 3c382cc to 3e8afbb Compare March 10, 2025 21:53
@achou11 achou11 force-pushed the ac/projects-hooks-tests branch from 3e8afbb to 3165747 Compare March 17, 2025 15:37
)

// Since useSingleProject() is Suspense-based, we need to simulate waiting for the data to initially resolve
await waitFor(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Based on react-hooks-testing-library tests I'm not sure this is how to test a suspense hook. The docs aren't great on this.

Copy link
Member Author

@achou11 achou11 Mar 18, 2025

Choose a reason for hiding this comment

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

yeah me neither. was mostly drawing inspiration from how React Query does its suspense hook tests: https://github.com/TanStack/query/blob/main/packages/react-query/src/__tests__/useSuspenseQuery.test.tsx

major difference is the DOM oriented approach as opposed to purely hook-based, which may be an important distinction

@gmaclennan
Copy link
Member

Took a quick look at this, but did not go too deep into the details. I left a comment questioning how to wait for suspense hooks - the docs are not clear at all.

This does smell of a race condition. One way to force a delay easily to expose any issues would be to over-ride the postMessage() function of the MessageChannel e.g.

const { port1, port2 } = new MessageChannel()
slowdown(port1)
slowdown(port2)
function slowdown(port) {
  const postMessageOrig = port.postMessage.bind(port)
  port.postMessage = (message) => {
    setTimeout(() => postMessageOrig(message), 200)
  }
}

That should ensure that everything is actually asynchronous, and ensure a loading state. Haven't tested it though, just an idea.

@gmaclennan
Copy link
Member

FYI I ran tests with the slowdown from above and got the same failing tests as seen in CI:

 ❯ test/hooks/maps.test.tsx (1 test | 1 failed) 586ms
   × basic read works 586ms
     → map style url hook updates after changing refresh token option: expected 'http://127.0.0.1:55042/maps/style.json' to not equal 'http://127.0.0.1:55042/maps/style.json'
 ✓ test/hooks/client.test.tsx (4 tests) 3254ms
   ✓ device info > basic read and write 1754ms
   ✓ is archive device > basic read and write 1351ms
 ❯ test/hooks/projects.test.tsx (10 tests | 2 failed | 3 skipped) 12228ms
   ✓ project creation works 599ms
   ✓ many projects > basic read 1374ms
   × single project > basic read 1381ms
     → created project id 2 exists
   ✓ single project > updates when a project changes settings 1824ms
   ✓ project settings > basic read and write 2188ms
   ✓ multiple members > gets own device 2663ms
   ↓ multiple members > updates after member joins
   ↓ multiple members > updates after member leaves
   × single member > gets own device 2197ms
     → single member read hook updates after updating own device info: expected { …(5) } to have deep property 'name' of 'device_1_updated', but got 'device_1'
   ↓ single member > gets other members after invite flow

Either the code has a bug, or the tests aren't quite right.

@achou11
Copy link
Member Author

achou11 commented Mar 19, 2025

Either the code has a bug, or the tests aren't quite right.

Yeah I suspect this too. I am curious about whether the Node version might be related. The issues seem to only happen when running on Node 20 in CI EDIT: nevermind, seems to happen on 18 too, although maybe less frequently

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.

3 participants