fix(NODE-3116): reschedule unreliable async interval first

[durran]

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

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[dariakp]

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"?

[durran]

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.

[dariakp]

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.

[durran]

Yeah I guess it's probably better to find a more meaningful way to test this and refactor the other tests as well.

[dariakp]

Note: we should decide if we still want to target 4.1 for this or update to 4.2