Skip to content
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.

Sign up for GitHub

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

test(codecatalyst): only fail test if message is more than a minute off. #6265

Merged
merged 2 commits into from
Dec 18, 2024
Merged

test(codecatalyst): only fail test if message is more than a minute off. #6265

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba95b24
pass if message is within 60 sec of correct
Hweinstock Dec 17, 2024
6157f8a
remove old check line
Hweinstock Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/core/src/testInteg/codecatalyst/devEnv.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,12 @@ describe('InactivityMessage', function () {
message: expectedMessages[i][0],
minute: expectedMessages[i][1],
}
assert.deepStrictEqual(actualMessages[i].message, expected.message)
// Avoid flakiness in the timing by looking within a minute rather than exact.
assert.ok(
Math.abs(actualMessages[i].minute - expected.minute) <= 1,
`Expected to be within 60 seconds of minute ${expected.minute}, but instead was at minute ${actualMessages[i].minute}`
)
assert.deepStrictEqual(actualMessages[i], expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

should drop this line, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was intending this line to check if the time the message popped up was within a minute of the expected time. Do you think it would be better to not check the time the message came up at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't remove "something", the test will still fail, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm making the check more lenient. The logs in the issue show that the message is popping up at minute 54, when we expect it at 55. If that happens, the current logic should pass now since its within one minute of the expected time.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly he's confused about why we aren't removing

assert.deepStrictEqual(actualMessages[i], expected)

since

assert.ok(Math.abs(actualMessages[i].minute - expected.minute) <= 1, 'Expected to be within 60 seconds of minute ${expected.minute}, but instead was at minute ${actualMessages[i].minute}')

is meant to replace it. If we kept the original check (assert.deepStrictEqual(actualMessages[i], expected)), wouldn't that check still fail since that piece of code is also checking the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my bad. Yes, we should remove that line. Sorry was looking at the wrong line and totally missed it.

}
}
Expand Down
Loading