-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
tools: fixup docs and run known_issues by default #21910
Conversation
tools/test.py
Outdated
@@ -1554,9 +1554,8 @@ def PrintCrashed(code): | |||
'gc', | |||
'internet', | |||
'pummel', | |||
'test-known-issues', | |||
'known_issues', |
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'm surprised known_issues
is in the ignore set. Any objection to removing 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.
(probably) fine with removing it, but might be best to do in a separate PR for visibility.
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.
But if this lands then won't we stop running known_issues
until the subsequent PR to remove it from this list lands?
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.
@richardlau Ah, true -- I'll remove known_issues
from the list in this PR :)
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, although I think we should be running known_issues, no?
Will land this as-is, and open up a separate PR re: |
CI before landing: https://ci.nodejs.org/job/node-test-pull-request/16006/ |
Nit: |
- Updates `test/README.md` with new suites - Fixes some outdated `IGNORED_SUITES` listings - Allows for `test/known_issues` suite to be run by default
Updated @richardlau @Trott, PTAL |
@@ -31,7 +35,7 @@ GitHub with the `autocrlf` git config flag set to true. | |||
|sequential |Yes |Various tests that are run sequentially.| | |||
|testpy | |Test configuration utility used by various test suites.| | |||
|tick-processor |No |Tests for the V8 tick processor integration. The tests are for the logic in ```lib/internal/v8_prof_processor.js``` and ```lib/internal/v8_prof_polyfill.js```. The tests confirm that the profile processor packages the correct set of scripts from V8 and introduces the correct platform specific logic.| | |||
|timers |No |Tests for [timing utilities](https://nodejs.org/api/timers.html) (```setTimeout``` and ```setInterval```).| | |||
|v8-updates |No |Tests for V8 performance integration.| | |||
|
|||
_When a new test directory is added, make sure to update the `CI_JS_SUITES` | |||
variable in the `Makefile` and the `js_test_suites` variable in |
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 bottom note can probably be removed too as it looks like both CI_JS_SUITES
and js_test_suites
are default
rather than a list of test directories. This can be done in another PR if you'd rather just land this PR as-is.
Landed in b1b2f7c, thank you for the reviews! |
- Updates `test/README.md` with new suites - Fixes some outdated `IGNORED_SUITES` listings - Allows for `test/known_issues` suite to be run by default PR-URL: #21910 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Depends on #22039 to land on |
- Updates `test/README.md` with new suites - Fixes some outdated `IGNORED_SUITES` listings - Allows for `test/known_issues` suite to be run by default PR-URL: #21910 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
test/README.md
with new suitesIGNORED_SUITES
listingsChecklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes