refactor: add timeout for race condition in heart test #5131
Conversation
|
✨ code-server docs for PR #5131 is ready! It will be updated on every commit.
|
Codecov Report
@@ Coverage Diff @@
## main #5131 +/- ##
=======================================
Coverage 71.73% 71.73%
=======================================
Files 30 30
Lines 1684 1684
Branches 374 374
=======================================
Hits 1208 1208
Misses 407 407
Partials 69 69 Continue to review full report at Codecov.
|
test/unit/node/heart.test.ts
Outdated
| // Check that the heart wrote to the heartbeatFilePath and overwrote our text | ||
| const fileContentsAfterBeat = await readFile(pathToFile, { encoding: "utf8" }) | ||
| expect(fileContentsAfterBeat).not.toBe(text) | ||
| // Make sure the modified timestamp was updated. | ||
| const fileStatusAfterEdit = await stat(pathToFile) | ||
| expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThan(fileStatusBeforeEdit.mtimeMs) | ||
| expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThanOrEqual(fileStatusBeforeEdit.mtimeMs) |
There was a problem hiding this comment.
It occurs to me that if we allow it to be equal then technically we are not testing anything because this will pass if the file is not modified. Any idea if this still flakes if we remove the equal?
There was a problem hiding this comment.
Maybe what we can do is set the modified time explicitly before the heartbeat (to something like 0 to simulate the file being really old) and then do the comparison?
There was a problem hiding this comment.
Oh good point. Let me remove the Equal and check. I also like the idea of explicitly setting. That gives us more control.
test/unit/node/heart.test.ts
Outdated
| const fileHandle = await open(pathToFile, "r+") | ||
| await fileHandle.utimes(0, 0) |
There was a problem hiding this comment.
Whoops, sorry I linked you to the wrong section that makes you open a file handle. I think you might have to close this as well? But it might be easier to use this one: https://nodejs.org/api/fs.html#fspromisesutimespath-atime-mtime
There was a problem hiding this comment.
Oh, no, that's my bad. I assumed you could only access utimes after opening the file and didn't bother checking the rest of the docs. Yeah, that would leak since I don't have logic to close it (I've run into that with Deno).
Nice!!! Our codebase thanks you for your attention-to-detail 😂
test/unit/node/heart.test.ts
Outdated
| @@ -1,5 +1,5 @@ | |||
| import { logger } from "@coder/logger" | |||
| import { readFile, writeFile, stat } from "fs/promises" | |||
| import { readFile, writeFile, stat, open, utimes } from "fs/promises" | |||
There was a problem hiding this comment.
Looks like we can remove open now.
There was a problem hiding this comment.
Oh whoops! VS Code should remove those for you 😛 Removing then merging
@code-asher was right. #5122 introduced a flakey test due to async logic in
beat. Added an artificial timeout in test to ensure no race conditions and test always passes.Had to re-run Build job here: https://github.com/coder/code-server/actions/runs/2228063603
Fixes N/A