-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(NODE-3116): reschedule unreliable async interval first #3006
Conversation
test/unit/utils.test.js
Outdated
@@ -140,8 +140,11 @@ describe('utils', function () { | |||
|
|||
// needs to happen on the third call because `wake` checks | |||
// the `currentTime` at the beginning of the function | |||
// The value of now() is not actually negative in the case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how this test works to test the "immediately schedule if the clock is unreliable"? I am having trouble following what the marks array is tracking. Also, the original test had an unexpected return time (i.e., a time in the past), why is that no longer a legitimate test case? Which clause in the utils code was it hitting originally? And does it definitely hit the new clause now? How do we differentiate between "executeAndReschedule" and just "reschedule"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a weird way of testing but I wanted to stay in line with the other tests in there. The marks are basically "marking" the time interval in which the repeatable callback was executed, and I admit it's not the clearest way to test. I changed this test because the title of the test, which was intended to test this specific scenario, wasn't actually executing the code path that was under test. The value simply being less than now was simply hitting the first block and returning. Once I switched to force the value to be negative, then the test failed until I moved the check we actually wanted to run up to be the first code branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that none of these tests are meaningfully asserting on the actual intent of the code: for example, if you comment out the entire block under the logic that is supposed to debounce multiple calls, the test above this one still passes, which tells me that we can't be confident that the current fix doesn't actually break some other desired behavior. I am worried that the current test structure is based on a flawed approach: we need to make sure that these tests actually isolate the test conditions, which means that if we are using just a sequence tracking the callback calls, we need to either make sure the sequence is unique to the expected code path - and make it clear why that sequence is unique to the expect path - or we need to make sure we are also asserting on what we expect to not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess it's probably better to find a more meaningful way to test this and refactor the other tests as well.
Note: we should decide if we still want to target 4.1 for this or update to 4.2 |
c9c586a
to
c5e1ec0
Compare
* fix(NODE-3116): reschedule unreliable async interval first Co-authored-by: Daria Pardue <[email protected]>
Description
When there is an unreliable clock we force async intervals to reschedule immediately.
What is changing?
The async interval check for negative clock times, like potentially AWS lambda, is moved to the first check so that we can potentially reschedule instead of just returning as was done previously.
Is there new documentation needed for these changes?
No.
Double check the following
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>