Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: run 'abort' suite on Windows #15056

Merged
merged 1 commit into from
Sep 3, 2017
Merged

test: run 'abort' suite on Windows #15056

merged 1 commit into from
Sep 3, 2017
+24 −19

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 28, 2017 •
edited
Loading

  • For now a C++ node::Abort() failure exits with code 3 on Windows

Fixes: #14012
Refs: #14013

CI: https://ci.nodejs.org/job/node-test-commit/12055/

/cc @nodejs/platform-windows @nodejs/testing

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test,process

@refack refack added flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Aug 28, 2017
@refack refack self-assigned this Aug 28, 2017
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Aug 28, 2017
@refack
Copy link
Contributor Author

refack commented Aug 28, 2017

@nodejs/build PTAL do you think aborting on windows will somehow create core dumps (via the unpredictable diagsvc)?

@refack refack force-pushed the run-abort-on-windows branch from f0940f6 to 7634941 Compare August 28, 2017 03:41
@refack
Copy link
Contributor Author

refack commented Aug 28, 2017

New CI: https://ci.nodejs.org/job/node-test-commit/12056/

@Trott
Copy link
Member

Trott commented Aug 28, 2017

Looks like abort/test-process-abort-exitcode needs a change (failing on Windows in CI).

addaleax
addaleax approved these changes Aug 30, 2017
View reviewed changes
test/abort/test-http-parser-consume.js
}));
} else {
const child = spawn(process.execPath, [__filename, 'child'],
{ stdio: 'inherit' });
// super-proces
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: process

Trott
Trott requested changes Sep 2, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a 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.

@refack
Copy link
Contributor Author

refack commented Sep 2, 2017

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)...

@Trott
Copy link
Member

Trott commented Sep 2, 2017

Windows does not generate core files by default.

Awesome. Then it's just the failing test...

@refack refack force-pushed the run-abort-on-windows branch from 7634941 to 98ba860 Compare September 2, 2017 23:25
@refack
Copy link
Contributor Author

refack commented Sep 2, 2017 •
edited
Loading

@Trott done
Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/11461/

Trott
Trott approved these changes Sep 3, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a 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

@refack refack force-pushed the run-abort-on-windows branch 2 times, most recently from 98d8db3 to 98ba860 Compare September 3, 2017 21:17
@refack
test,process: run 'abort' suite on Windows
233d1e2 
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>
@refack refack force-pushed the run-abort-on-windows branch from 98ba860 to 233d1e2 Compare September 3, 2017 21:20
@refack refack merged commit 233d1e2 into nodejs:master Sep 3, 2017
@refack refack mentioned this pull request Sep 3, 2017
Closed
@refack refack deleted the run-abort-on-windows branch September 4, 2017 21:53
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@addaleax
test,process: run 'abort' suite on Windows
49da82c 
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>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@refack @MylesBorins
test,process: run 'abort' suite on Windows
c3e4c12 
PR-URL: #15056
Fixes: #14012
Refs: #14013
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
@MylesBorins
Copy link
Contributor

This was relying on a semver major change. If possible in future please appropriately tag dont-land, thanks.

@richardlau
Copy link
Member

It isn't relying per se. #13947 should have changed test-process-abort-exitcode.js on Windows to match the new behaviour -- Presumably this was missed because prior to this PR the abort tests weren't included in the CI runs.

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).

@MylesBorins
Copy link
Contributor

@richardlau please feel free to submit a backport if you think a partial change makes sense on 8.x

@MylesBorins MylesBorins mentioned this pull request Sep 11, 2017
Closed
5 tasks
@refack refack removed their assignment Sep 18, 2017
@refack refack mentioned this pull request Sep 18, 2017
Closed
3 tasks
@refack
Copy link
Contributor Author

refack commented Sep 18, 2017

@MylesBorins Backport PR #15460 (since it's build/test you could land it irrespective of a new release)

refack added a commit to refack/node that referenced this pull request Sep 23, 2017
@refack
test,process: run 'abort' suite on Windows
26f2755 
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>
jasnell pushed a commit that referenced this pull request Sep 25, 2017
@refack @jasnell
test,process: run 'abort' suite on Windows
e014983 
PR-URL: #15056
Fixes: #14012
Refs: #14013
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@refack refack mentioned this pull request Oct 24, 2017
Closed
2 tasks
@refack
Copy link
Contributor Author

refack commented Oct 24, 2017

Backport PR: #16442

refack added a commit to refack/node that referenced this pull request Oct 24, 2017
@refack
test,process: run 'abort' suite on Windows
d862064 
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>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@refack @MylesBorins
test,process: run 'abort' suite on Windows
54286db 
Backport-PR-URL: #16442
PR-URL: #15056
Fixes: #14012
Refs: #14013
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@refack @MylesBorins
test,process: run 'abort' suite on Windows
3e96c85 
Backport-PR-URL: #16442
PR-URL: #15056
Fixes: #14012
Refs: #14013
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2017
@refack @MylesBorins
test,process: run 'abort' suite on Windows
e0c4f0b 
Backport-PR-URL: #16442
PR-URL: #15056
Fixes: #14012
Refs: #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 referenced Nov 3, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@gireeshpunathil gireeshpunathil Awaiting requested review from gireeshpunathil

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: implement a way to regularly test abort behaviour on all platform
7 participants
@refack @Trott @MylesBorins @richardlau @jasnell @addaleax @nodejs-github-bot