test: remove timers-blocking-callback

[Fishrock123]

If 5aac4c4 is effectively reverted, reintroducing the bug this was intended to catch, many (50+) tests time out, rendering this test redundant and unnecessary.

in particular, the following timer tests catch an effective revert of 5aac4c4:

not ok 21 parallel/test-timers-api-refs not ok 22 parallel/test-timers-args not ok 23 parallel/test-timers-destroyed not ok 25 parallel/test-timers-nested not ok 26 parallel/test-timers-interval-throw not ok 28 parallel/test-timers-non-integer-delay not ok 32 parallel/test-timers-ordering not ok 33 parallel/test-timers-refresh not ok 34 parallel/test-timers-refresh-in-callback not ok 35 parallel/test-timers-reset-process-domain-on-throw not ok 40 parallel/test-timers-timeout-to-interval not ok 41 parallel/test-timers-uncaught-exception not ok 42 parallel/test-timers-timeout-with-non-integer not ok 43 parallel/test-timers-unenroll-unref-interval not ok 44 parallel/test-timers-unref not ok 45 parallel/test-timers-unref-active not ok 46 parallel/test-timers-unrefd-interval-still-fires not ok 47 parallel/test-timers-unrefed-in-callback not ok 48 parallel/test-timers-user-call not ok 49 parallel/test-timers-zero-timeout 

Refs: #21781

---

To be clear, this is the patch I tested with. Maybe @apapirovski can verify if that is a correct-ish revert.

It does also make both sequential tests fail:

not ok 1 sequential/test-timers-block-eventloop not ok 2 sequential/test-timers-set-interval-excludes-callback-duration 

diff --git a/lib/internal/timers.js b/lib/internal/timers.js index bb80f57ee2..026bb0aef9 100644 --- a/lib/internal/timers.js+++ b/lib/internal/timers.js@@ -73,7 +73,7 @@ // have shown to be trivial in comparison to other timers architectures. const{- MathMax,+ // MathMax, MathTrunc, NumberMIN_SAFE_INTEGER, ObjectCreate, @@ -507,7 +507,8 @@ function getTimerCallbacks(runNextTicks){// Check if this loop iteration is too early for the next timer. // This happens if there are more timers scheduled for later in the list. if (diff < msecs){- list.expiry = MathMax(timer._idleStart + msecs, now + 1);+ list.expiry = msecs - diff;+ // MathMax(timer._idleStart + msecs, now + 1); list.id = timerListId++; timerListQueue.percolateDown(1); debug('%d list wait because diff is %d', msecs, diff);

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[Trott]

@Fishrock123 Is this the TL;DR?:

• Removing this test because it is flaky/unreliable
• We don't need the test because if we ever reintroduce the bug the test was written to catch, there are other tests that will not pass

[Fishrock123]

Yes, my assessment is we can just remove this because the behaviour it was designed to catch is so broken that large parts of the current test suit will time out (fail).

[apapirovski]

Sorry, I didn't say more earlier but I agree that this test is redundant. Tbh I've previously considered removing this myself so I'm strongly in favour of this.

[Fishrock123]

Amended the commit to remove the flaky test status. Can I get a second approval here so we can be done with this?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30982/

[Trott]

Landed in f8d5474