-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
test: move tick-processor tests to own directory #9506
Conversation
The tick-processor tests are inherently non-deterministic. They therefore have false negatives from time to time. They also sometimes leave extra processes running. Move them to their own directory until these issues are sorted. Note that this means that the tests will not be run in CI. Like the inspector tests and other tests, they will have to be run manually when they are wanted.
Fixes: #8725 sorta kinda I guess |
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.
@Trott Could you add something to ./test/README.md documenting this?
Otherwise LGTM.
I feel that tests that aren't run as part of CI tend to stop being up to date, but I'm not sure what the best solution to that would be. Maybe a less frequently run ci target that runs everything?
@gibfahn OK, README info added. I included more details than we usually include based on text from @matthewloring so cc'ing him in the hope he can give it a quick review to confirm that I didn't introduce any errors in imprecision. |
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.
LGTM with a nit
@@ -122,6 +122,17 @@ Various tests that are run sequentially. | |||
|
|||
Test configuration utility used by various test suites. | |||
|
|||
### inspector |
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.
This should probably say tick processor
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.
Whoops! You are, of course, correct. Fixed. Thanks!
@@ -21,7 +21,7 @@ const base = require('./tick-processor-base.js'); | |||
// Unknown checked for to prevent flakiness, if pattern is not found, | |||
// then a large number of unknown ticks should be present | |||
base.runTest({ | |||
pattern: /LazyCompile.*\[eval\]:1|.*% UNKNOWN/, | |||
pattern: /LazyCompile.*\[eval]:1|.*% UNKNOWN/, |
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.
Why was this removed?
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.
]
does not need to be escaped in a regular expression unless it occurs in a character class. I've been removing unnecessary escaping (especially in regular expressions) when I touch a file lately. I can put it back if you or anyone else thinks the regular expression is easier to read with it.
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.
I personally find the explicit escaping more readable but I don't feel too strongly about it.
The tick-processor tests are inherently non-deterministic. They therefore have false negatives from time to time. They also sometimes leave extra processes running. Move them to their own directory until these issues are sorted. Note that this means that the tests will not be run in CI. Like the inspector tests and other tests, they will have to be run manually when they are wanted. PR-URL: nodejs#9506 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Landed in eed8b05 |
The tick-processor tests are inherently non-deterministic. They therefore have false negatives from time to time. They also sometimes leave extra processes running. Move them to their own directory until these issues are sorted. Note that this means that the tests will not be run in CI. Like the inspector tests and other tests, they will have to be run manually when they are wanted. PR-URL: #9506 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
The tick-processor tests are inherently non-deterministic. They therefore have false negatives from time to time. They also sometimes leave extra processes running. Move them to their own directory until these issues are sorted. Note that this means that the tests will not be run in CI. Like the inspector tests and other tests, they will have to be run manually when they are wanted. PR-URL: #9506 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
hey @Trott this didn't land cleanly on |
The tick-processor tests are inherently non-deterministic. They therefore have false negatives from time to time. They also sometimes leave extra processes running. Move them to their own directory until these issues are sorted. Note that this means that the tests will not be run in CI. Like the inspector tests and other tests, they will have to be run manually when they are wanted. PR-URL: #9506 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test
Description of change
The tick-processor tests are inherently non-deterministic. They
therefore have false negatives from time to time. They also
sometimes leave extra processes running.
Move them to their own directory until these issues are sorted. Note
that this means that the tests will not be run in CI. Like the inspector
tests and other tests, they will have to be run manually when they are
wanted.
/cc @nodejs/build @nodejs/testing @matthewloring @indutny