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