FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove timers-blocking-callback by Fishrock123 · Pull Request #32870 · nodejs/node · GitHub

/ node Public

test: remove timers-blocking-callback - #32870

Closed
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:remove-test-timers-blocking-callback
ConversationCommits (1)ChecksFiles changed
Closed

test: remove timers-blocking-callback#32870
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:remove-test-timers-blocking-callback

Conversation

Fishrock123 commented Apr 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Fishrock123 added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. test Issues and PRs related to the tests. labels Apr 15, 2020
AshCripps mentioned this pull request Apr 16, 2020
Merged
4 tasks

Trott commented Apr 17, 2020

Copy link
Copy Markdown
Member

@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 commented Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Apr 22, 2020

Copy link
Copy Markdown
Contributor

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.

test: remove timers-blocking-callback
2d10f13 
If the bug this test is intented to catch is reintroduced, or if 5aac4c4 is effectively reverted, 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: nodejs#21781
Fishrock123 force-pushed the remove-test-timers-blocking-callback branch from 8665b1c to 2d10f13 Compare April 25, 2020 02:11

Fishrock123 commented Apr 25, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Apr 25, 2020

Copy link
Copy Markdown
Collaborator

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

Trott pushed a commit that referenced this pull request Apr 26, 2020
test: remove timers-blocking-callback
f8d5474 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott commented Apr 26, 2020

Copy link
Copy Markdown
Member

Landed in f8d5474

Trott closed this Apr 26, 2020
Fishrock123 deleted the remove-test-timers-blocking-callback branch April 26, 2020 06:49
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
test: remove timers-blocking-callback
242dfdd 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
test: remove timers-blocking-callback
5bdb401 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
test: remove timers-blocking-callback
790c453 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
test: remove timers-blocking-callback
8fad112 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2020
test: remove timers-blocking-callback
71fa8d7 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos mentioned this pull request May 2, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
test: remove timers-blocking-callback
8352045 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 15, 2020
test: remove timers-blocking-callback
7a109fe 
If the bug this test is intented to catch is reintroduced, or if
5aac4c4 is effectively reverted, 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

PR-URL: #32870
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau mentioned this pull request Jul 15, 2020
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
MylesBorins Awaiting requested review from MylesBorins
+2 more reviewers
apapirovski apapirovski approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL