Prep for timing out the initial fetch. #4753
Conversation
| @@ -82,7 +83,10 @@ describe('fetchActions', () => { | |||
|
|
|||
| describe('tryFetch', () => { | |||
| test('resolves any promise, if there is no exception', async () => { | |||
| const tryFetchFunc = () => new Promise(resolve => setTimeout(() => resolve('hello'), 100)); | |||
| const tryFetchFunc = async () => { | |||
| await sleep(10); | |||
There was a problem hiding this comment.
FWIW, Re: the commit message, I don't think 10ms is a very realistic API request response time, although I don think it's reasonable to use in a test. You'd need to be quite close to the server, and calling a fast API endpoint to get 10ms.
There was a problem hiding this comment.
Yeah; I see that I didn't back up that statement with anything. 🤔 I think I was going for "more realistic than no sleep at all (what the error case was doing) but still wouldn't slow down the tests as noticeably as 100ms." I'll fix it.
(hmm also since we often ask Jest to run lots of tests at a time, and Jest runs several tests in parallel, and it still regularly takes over a second on my machine, I think 100ms would not generally slow down the Jest tests even without fake timers)
Could you please confirm that you meant "do" for "don", or did you mean "don't"? I'm thinking of changing to 100ms instead of 10s; still optimistic perhaps (see below) but more realistic than 10ms.
There was a problem hiding this comment.
Looks like GET /server_settings on CZO is taking ~300ms for me.
There was a problem hiding this comment.
Ah, yeah, I meant "do" — 10ms seems fine, it's hard to imagine what wouldn't break at 10ms but would at 100ms.
| @@ -116,13 +116,19 @@ describe('fetchActions', () => { | |||
| jest.runAllTimers(); | |||
| }); | |||
|
|
|||
| test('retries a call, if there is an exception', async () => { | |||
| test('retries a call if there is a non-client error', async () => { | |||
There was a problem hiding this comment.
Could you add something like the commit message as a comment here? I think that's more useful.
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.
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 (before this commit) was passing isn't good -- it means a basically arbitrary, unexpected kind of error will let the retry loop continue without propagating to `tryFetch`'s caller. Instead, if something unexpected like that happens, we should fail early (and stop wasting the user's time) by breaking out of the retry loop and having the error propagate to the caller. We plan to fix that in a later series of commits, and add a test case with an error like that. But for now, test that we get the right behavior with representative inputs.
chrisbobbe
force-pushed
the
pr-initial-fetch-timeout-prep
branch
from
2dc89ef
to
9f54560
Compare
|
Thanks for the review! Merged, after making those changes. |
|
Thanks @chrisbobbe and @WesleyAC ! I've just read through this, and have just two small comments:
|
This supersedes the first part of #4193, which has grown unwieldy. I'm hoping this smaller PR will be easier to review, and then I'll follow it up with another PR (EDIT: #4754) to finish the work of #4193. 🙂
Related: #4165