Fix stoppable flakiness by mocking timers #7117
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e951c9c:
|
| const gracefully = await stopper.stop(500); | ||
| const [gracefully] = await Promise.all([ | ||
| stopper.stop(500), | ||
| jest.advanceTimersByTime(500), |
There was a problem hiding this comment.
Doesn't this kinda run the two functions in parallel? How do we know it won't advance the timers before we call stop(500)?
| const elapsed = Date.now() - start; | ||
| expect(elapsed).toBeGreaterThanOrEqual(450); | ||
| expect(elapsed).toBeLessThanOrEqual(550); | ||
| expect(elapsed).toBe(500); |
There was a problem hiding this comment.
I might be wrong but I think this isn't actually testing anything any more.
Originally, what the test showed was that in this case (where there are a couple requests "hanging" because the response hasn't "end"ed), it would not immediately kill the requests, but that we would in fact have to wait 500 ms. So this could fail by getting a really small value for elapsed if it didn't actually wait the whole grace period.
But in this case, if I understand correctly, this is merely just testing that the jest.advanceTimersByTime ran. So it can't really fail?
Here's an idea. Stopper is a fully internal class so we can do whatever we want with it. What if we replaced stopGracePeriodMillis with an AbortSignal (we even have a dependency on node-abort-controller as of Monday...). So the setTimeout would occur in the drainServer hook in ApolloServerPluginDrainHttpServer but Stopper wouldn't care about time any more (it would just do a signal.on('abort') or whatever instead of a setTimeout).
Then the tests wouldn't have to mock out time at all: they would be able to deterministically show that stopper.stop(signal) doesn't return at all until the test calls controller.abort().
The API would basically be that stop closes the listening socket, starts actively closing connections when they go idle, and waits until all connections are closed. In addition, it takes an AbortSignal, and if that signals abort then it force closes the rest of them.
(Other tests have similar concerns.)
There was a problem hiding this comment.
(Note that there is a test that uses an external process; for this one you could send a SIGUSR1 or something to trigger the abortcontroller?)
There was a problem hiding this comment.
I implemented this suggestion in #7232
|
Superseded by #7232 |
Fixes #6963