refactor: add timeout for race condition in heart test #5131

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.

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

Merged

jsjoeio merged 4 commits into main from jsjoeio/fix-flake

Apr 26, 2022

Merged

refactor: add timeout for race condition in heart test #5131

jsjoeio merged 4 commits into main from jsjoeio/fix-flake

Apr 26, 2022

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Apr 26, 2022

@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


 refactor: add timeout for race condition in heart test

b14024f

@jsjoeio jsjoeio added the testing label

Apr 26, 2022

@jsjoeio jsjoeio added this to the April 2022 milestone

Apr 26, 2022

@jsjoeio jsjoeio requested a review from a team

April 26, 2022 16:45

@jsjoeio jsjoeio self-assigned this

Apr 26, 2022

@github-actions

Copy link

github-actions bot commented Apr 26, 2022 •

edited

Loading

✨ code-server docs for PR #5131 is ready! It will be updated on every commit.

Host: https://coder.com/docs/code-server/5cbc558
Last deploy status: success
Commit: 4ad1d02
Workflow status: https://github.com/coder/code-server/actions/runs/2228259663

@codecov

Copy link

codecov bot commented Apr 26, 2022 •

edited

Loading

Codecov Report

Merging #5131 (4ad1d02) into main (18ff996) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ 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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18ff996...4ad1d02. Read the comment docs.

@jsjoeio jsjoeio temporarily deployed to npm

April 26, 2022 16:50

Inactive

@github-actions

Copy link

github-actions bot commented Apr 26, 2022 •

edited

Loading

✨ code-server dev build published to npm for PR #5131!

Last publish status: success
Commit: 4ad1d02

To install in a local project, run:

npm install @coder/code-server-pr@5131

To install globally, run:

npm install -g @coder/code-server-pr@5131

code-asher

code-asher reviewed

Apr 26, 2022

View reviewed changes

test/unit/node/heart.test.ts Outdated

// Make sure the modified timestamp was updated.

const fileStatusAfterEdit = await stat(pathToFile)

expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThan(fileStatusBeforeEdit.mtimeMs)

expect(fileStatusAfterEdit.mtimeMs).toBeGreaterThanOrEqual(fileStatusBeforeEdit.mtimeMs)

Copy link

Member

@code-asher code-asher Apr 26, 2022

Choose a reason for hiding this comment

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

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?

Copy link

Member

@code-asher code-asher Apr 26, 2022 •

edited

Loading

Choose a reason for hiding this comment

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

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?

https://nodejs.org/api/fs.html#filehandleutimesatime-mtime

Copy link

Contributor Author

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

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

Oh good point. Let me remove the Equal and check. I also like the idea of explicitly setting. That gives us more control.


 fixup!: set mtime to 0 and check for update

e203cc3

@jsjoeio jsjoeio requested a review from code-asher

April 26, 2022 17:01

@jsjoeio jsjoeio temporarily deployed to npm

April 26, 2022 17:07

Inactive

code-asher

code-asher reviewed

Apr 26, 2022

View reviewed changes

test/unit/node/heart.test.ts Outdated

Comment on lines 39 to 40

const fileHandle = await open(pathToFile, "r+")

await fileHandle.utimes(0, 0)

Copy link

Member

@code-asher code-asher Apr 26, 2022 •

edited

Loading

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

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

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 😂


 fixup!: use utimes directly instead of file open

b719d2a

code-asher

code-asher approved these changes

Apr 26, 2022

View reviewed changes

Copy link

Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Awesome! This seems like it should be solid.

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"

Copy link

Member

@code-asher code-asher Apr 26, 2022

Choose a reason for hiding this comment

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

Looks like we can remove open now.

Copy link

Contributor Author

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

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

Oh whoops! VS Code should remove those for you 😛 Removing then merging


 fixup!: remove import

4ad1d02

@jsjoeio jsjoeio enabled auto-merge (squash)

April 26, 2022 17:17

@jsjoeio jsjoeio temporarily deployed to npm

April 26, 2022 17:22

Inactive

@jsjoeio jsjoeio merged commit 683412c into main

Apr 26, 2022

@jsjoeio jsjoeio deleted the jsjoeio/fix-flake branch

April 26, 2022 17:39

@jsjoeio jsjoeio mentioned this pull request

Apr 28, 2022

refactor(heart): bind class methods and make beat async #5142

Merged

Labels

testing

2 participants

@jsjoeio @code-asher

refactor: add timeout for race condition in heart test #5131

refactor: add timeout for race condition in heart test #5131

Uh oh!

Conversation

@jsjoeio jsjoeio commented Apr 26, 2022

Uh oh!

github-actions bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@code-asher code-asher Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 26, 2022 •

edited

Loading

codecov bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

@code-asher code-asher Apr 26, 2022 •

edited

Loading

@code-asher code-asher Apr 26, 2022 •

edited

Loading