-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
test: run 'abort' suite on Windows #15056
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
Conversation
|
@nodejs/build PTAL do you think aborting on windows will somehow create core dumps (via the unpredictable |
f0940f6 to
7634941
Compare
|
Looks like |
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.
typo: process
Trott
left a comment
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.
Looks like there's a relevant test failure. Also very interested in whether we can turn off core files in test.py for Windows or not.
Windows does not generate core files by default. So it's actually a matter to finding a way to turn them on (if we find a way to validate them)... |
Awesome. Then it's just the failing test... |
7634941 to
98ba860
Compare
|
@Trott done |
Trott
left a comment
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 if CI is green
98d8db3 to
98ba860
Compare
PR-URL: nodejs#15056 Fixes: nodejs#14012 Refs: nodejs#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
98ba860 to
233d1e2
Compare
PR-URL: nodejs/node#15056 Fixes: nodejs/node#14012 Refs: nodejs/node#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
This was relying on a semver major change. If possible in future please appropriately tag |
|
It isn't relying per se. #13947 should have changed Perhaps this PR should have been two commits -- one to unbreak the tests (i.e. the bit that is dependent on the semver-major change) and a separate commit to enable the abort tests in the list of testsuites to run on Windows (which should be backported if #14013 is backported otherwise there'll be a disparity between the tests that are run on Windows vs. everywhere else). |
|
@richardlau please feel free to submit a backport if you think a partial change makes sense on 8.x |
|
@MylesBorins Backport PR #15460 (since it's build/test you could land it irrespective of a new release) |
PR-URL: nodejs#15056 Fixes: nodejs#14012 Refs: nodejs#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Should this be backported to |
|
Backport PR: #16442 |
PR-URL: nodejs#15056 Fixes: nodejs#14012 Refs: nodejs#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
For now a C++node::Abort()failure exits with code 3 on WindowsFixes: #14012
Refs: #14013
CI: https://ci.nodejs.org/job/node-test-commit/12055/
/cc @nodejs/platform-windows @nodejs/testing
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test,process