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(usePersistedStateReducer.test): Fix flaky test of push tab retry #2191

Merged
merged 3 commits into from
Aug 22, 2023
Merged

fix(usePersistedStateReducer.test): Fix flaky test of push tab retry #2191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff2a667
test(usePersistedStateReducerTest): wait for all promises to resolve …
KaylaBrady Aug 17, 2023
7acc57d
refactor(usePersistedStateReducer): store remaining retries with ref …
KaylaBrady Aug 17, 2023
e039e78
test(usePersistedStateReducer): await act in other retry logic test
KaylaBrady Aug 17, 2023
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
21 changes: 10 additions & 11 deletions assets/src/hooks/usePersistedStateReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useReducer, useState } from "react"
import { useEffect, useReducer, useRef } from "react"
import { putRouteTabs } from "../api"
import appData from "../appData"
import { loadState, saveState } from "../localStorage"
Expand Down Expand Up @@ -48,32 +48,31 @@ const usePersistedStateReducer = (): [State, Dispatch] => {

const { routeTabsToPush, routeTabsPushInProgress } = state

const [routeTabsPushRetriesLeft, setRouteTabsPushRetriesLeft] =
useState(routeTabsPushRetries)
const routeTabsPushRetriesLeft = useRef(routeTabsPushRetries)
Copy link
Member

Choose a reason for hiding this comment

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

question: I'm wondering if it would even be possible to simply use a local variable in usePersistedStateReducer rather than a ref, though I haven't thought it all the way through yet and my naive attempt at doing it locally isn't working. This logic is really hard to reason about and I'm hoping there's some way to simplify it a bit further but it might not be possible while fully obeying all of the rules of hooks and everything.

Copy link
Contributor Author

@KaylaBrady KaylaBrady Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't been able to get it working using a local variable either - doing some logging in the useEffect, it seems that as a local variable, the value never is decremented. My hunch is that it has do with the local variable being reset when the state returned by useReducer changes and forces a re-render, whereas useRef will persist across re-renders


/* eslint-disable react-hooks/exhaustive-deps */
useEffect(() => {
if (routeTabsToPush && !routeTabsPushInProgress) {
const currentTriesLeft = routeTabsPushRetriesLeft.current
dispatch(startingRouteTabsPush())
putRouteTabs(routeTabsToPush)
.then((response) => {
if (response.ok) {
setRouteTabsPushRetriesLeft(routeTabsPushRetries)
routeTabsPushRetriesLeft.current = routeTabsPushRetries
dispatch(routeTabsPushComplete())
} else if (routeTabsPushRetriesLeft > 0) {
setRouteTabsPushRetriesLeft(routeTabsPushRetriesLeft - 1)
} else if (currentTriesLeft > 0) {
routeTabsPushRetriesLeft.current = currentTriesLeft - 1
dispatch(retryRouteTabsPushIfNotOutdated(routeTabsToPush))
} else {
setRouteTabsPushRetriesLeft(routeTabsPushRetries)
routeTabsPushRetriesLeft.current = routeTabsPushRetries
dispatch(routeTabsPushComplete())
}
})
.catch(() => {
if (routeTabsPushRetriesLeft > 0) {
setRouteTabsPushRetriesLeft(routeTabsPushRetriesLeft - 1)
if (currentTriesLeft > 0) {
routeTabsPushRetriesLeft.current = currentTriesLeft - 1
dispatch(retryRouteTabsPushIfNotOutdated(routeTabsToPush))
} else {
setRouteTabsPushRetriesLeft(routeTabsPushRetries)
routeTabsPushRetriesLeft.current = routeTabsPushRetries
dispatch(routeTabsPushComplete())
}
})
Expand Down
26 changes: 5 additions & 21 deletions assets/tests/hooks/usePersistedStateReducer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
beforeEach,
afterAll,
} from "@jest/globals"
import { act, renderHook, waitFor } from "@testing-library/react"
import { act, renderHook } from "@testing-library/react"
import { putUserSetting, putRouteTabs } from "../../src/api"
import appData from "../../src/appData"
import usePersistedStateReducer, {
Expand Down Expand Up @@ -320,24 +320,13 @@ describe("usePersistedStateReducer", () => {
.mockImplementationOnce(() => fakePromise)
.mockImplementationOnce(() => fakePromise)

const firstValue = result.current

act(() => {
await act(async () => {
dispatch(createRouteTab())
})
await waitFor(() => {
expect(result.current).not.toBe(firstValue)
})

// wait for changes from dispatch call in catch handler
const secondValue = result.current

await waitFor(() => {
expect(result.current).not.toBe(secondValue)
})

const [state] = result.current

expect(putRouteTabs).toHaveBeenCalledTimes(3)
expect(state.routeTabs).toMatchObject([
{
ordering: 0,
Expand All @@ -347,7 +336,6 @@ describe("usePersistedStateReducer", () => {
expect(state.routeTabsToPush).toBeNull()
expect(state.routeTabsToPushNext).toBeNull()
expect(state.routeTabsPushInProgress).toEqual(false)
expect(putRouteTabs).toHaveBeenCalledTimes(3)
})

test("retries at most two more times, with final failure being a client error", async () => {
Expand All @@ -363,16 +351,13 @@ describe("usePersistedStateReducer", () => {
.mockImplementationOnce(() => fakePromise)
.mockImplementationOnce(() => fakePromise)

act(() => {
await act(async () => {
dispatch(createRouteTab())
})
const initialValue = result.current
await waitFor(() => {
expect(result.current).not.toBe(initialValue)
})

const [state] = result.current

expect(putRouteTabs).toHaveBeenCalledTimes(3)
expect(state.routeTabs).toMatchObject([
{
ordering: 0,
Expand All @@ -382,7 +367,6 @@ describe("usePersistedStateReducer", () => {
expect(state.routeTabsToPush).toBeNull()
expect(state.routeTabsToPushNext).toBeNull()
expect(state.routeTabsPushInProgress).toEqual(false)
expect(putRouteTabs).toHaveBeenCalledTimes(3)
})
})

Expand Down