Initial fetch timeout (with working tests) #4193

Soon, there'll be a kind of annoying thing we have to do with custom Jest presets, and we want to have a good place to put a single long-ish comment about it. So, make it so that we only have to say `preset: ` once, and plan to put the comment just above that.

And remove a blank line that looks like it shouldn't be there.

We don't actually use this response (hence why we choose the boring mock), but it's cleaner not to have unhandled promise rejections from the `.json` property being missing on the response.

This is our more "batteries-included" Redux state, with standard example data like `selfUser`; see its jsdoc.

This is much easier than forking React Native, as we considered in zulip/react-native#5. See jestjs/jest#10221 (comment).

`tryFetch` requires the passed function to have a `Promise` return type; it's unnecessary to check its behavior when the passed function doesn't have a `Promise` return type.

So it's easier to compare the inputs we're testing. 10ms is a realistic amount of time for an API request to take. It's also less than 100ms, which had been used for one of these. So, our tests will run a bit faster (and they'll run a lot faster when we switch over to fake timers soon).

And make the tests more rigorous while we're at it. When we add a timeout to `tryFetch`, we'll want to use fake timers so that the tests don't try to do inconvenient things like waiting a whole minute for something to happen.

The fact that this test passes isn't good -- it means a basically arbitrary, unexpected kind of error will let the retry loop continue without propagating to `tryFetch`'s caller. We'll fix that logic soon, and add a test case with an error like that. But for now, test that we get the right behavior with representative inputs.

We'll use this in `tryFetch`, in an upcoming commit, so that we can interrupt in-progress attempts to contact a server when they take too long. See discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907693

To be dispatched when it doesn't seem like we'll get a response from the server, or when the server responds with a 5xx error. It navigates to the 'accounts' screen so a user can try a different account and server. Logging out wouldn't be good; the credentials may be perfectly fine, and we'd like to keep them around to try again later. It sets `needsInitialFetch` to `false` [1], just like `INITIAL_FETCH_COMPLETE`, while retaining a different meaning than that action (i.e., that the fetch was aborted instead of completed). Setting `needsInitialFetch` to false is necessary to ensure that a subsequent initial fetch can be triggered when we want it to be. As also noted in 7caa4d0, `needsInitialFetch` is "edge-triggered". (That edge-triggering logic seems complex and fragile, and it would be nice to fix that.) See also discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907591

Change the condition for exiting the retry loop from `isClientError(e)` to `!isServerError(e)`.

So far, `tryFetch`'s only callers are in the initial fetch; so, add handling for the `TimeoutError` there. The choice of value for `requestLongTimeoutMs` comes from a suggestion in zulip#4165's description: > As a minimal version, if the initial fetch takes a completely > unreasonable amount of time (maybe one minute), we should give up > and take you to the account-picker screen so you can try a > different account and server. Fixes: zulip#4165

As Greg points out [1], this makes the most sense conceptually; it's happening at the bottom of the loop, just before a new iteration starts. The `return` in the `try` block is enough to ensure that this wait won't interfere with a successful fetch. [1]: zulip#4166 (comment)

Greg points out that the initial fetch isn't actually a place where we want to retry on 5xx errors [1]: > Ah, I think in #M4165 the point is that if the server isn't > responding, we want to give you the option to go choose some other > account. The context there is that we're in the initial fetch, so > showing the loading screen, and as long as we're doing that > there's no other UI. > So yeah, I think basically we don't want to do any retrying here. > Instead we can kick you to the account-picker screen, with a toast > or something to indicate an error, and then you might manually > retry a time or two or you might bail and switch to some other > account. > And in particular if you didn't even want to be using that account > anymore -- maybe you even know that it's a server which is > permanently shut down, but it just happened to be the last one > you'd been using in the app and so it's the one we tried loading > data from on startup -- then you can go use whatever other account > you were actually opening the app to use. This does mean that `tryFetch` now has no callsites. It's a useful function, so we'll add a useful callsite soon so the function still gets exercised and maintained as appropriate. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/1178689

A `TimeoutError` will be handled the same way other errors in `fetchMessages` are handled; if it's a timeout in the fetch `ChatScreen` does on mount, `ChatScreen` will show the `FetchError` component we set up in zulip#4205. There's also been a passing mention on CZO of doing a timeout like this [1]: > After a long time, probably like a minute, we'll want that [...] > fetch to time out and fail in any case. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M4156.20Message.20List.20placeholders/near/950853

And add that message and the existing message to messages_en.json; looks like we forgot to add the existing one.

Use Jest's "modern" fake timers instead of our Lolex wrapper. Also, remove one `describe` block for tests that examine an edge-case safety feature that we built into our Lolex wrapper, but that doesn't seem to exist in Jest. Ah, well.

Use Jest's "modern" fake timers instead of our Lolex wrapper.

We've entirely switched over to Jest's "modern" fake timers, which landed in jestjs/jest#7776.

Also, remove several now-unnecessary calls of `jest.useFakeTimers('modern')`, but keep a few assertions that the "modern" timers are actually being used. In particular, our `jestSetup` is a central place where we make the assertion. Not only is it good to check that we still intentionally set the "modern" implementation, but we want to make sure that the setting is correctly applied. See the note in fb23341 about it being silently not applied until we added @jest/source-map as a direct dependency. We have an ESLint rule, from 2faad06, preventing imports from '**/__tests__/**'; the rule is active in all files not matching that same pattern. Add an additional override so that we can make the "modern"-timers assertion from within `jest/jestSetup.js`.

Follow and delete a code comment at the top of `backoffMachine-test`, suggesting that we move these tests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial fetch timeout (with working tests) #4193

Initial fetch timeout (with working tests) #4193

Commits on May 19, 2021