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.

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: fix flakyness with `yes.exe` #12821

Closed
wants to merge 0 commits into from

Conversation

@refack
Copy link
Member

commented May 4, 2017

Kills yes.exe with fire cause it's evil

Fixes: #12817
Ref: #12658

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, benchmark

@refack

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

/cc @Trott
Maybe we should find something more congenial then yes.exe
💡
🤓
Maybe a node script?

@Trott
Trott approved these changes May 4, 2017
Copy link
Member

left a comment

LGTM if CI is green.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

@cjihrig
cjihrig approved these changes May 4, 2017
child.kill();
if (process.platform === 'win32') {
// Sometimes there's a yes.exe process left hanging around on Windows...
child_process.execSync(`taskkill /f /t /pid ${child.pid}`);

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung May 4, 2017

Member

CI is failing...

ReferenceError: child_process is not defined

Only the child_process.spawn is stored in a variable above..

This comment has been minimized.

Copy link
@refack

refack May 4, 2017

Author Member

Fixed 😞
I should keep the late night commits to a minimum...

const os = require('os');
const child_process = require('child_process');

This comment has been minimized.

Copy link
@refack

refack May 4, 2017

Author Member

As per test style guide. Even though it's a benchmark IMHO still a good idea.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

@refack refack referenced this pull request May 4, 2017
2 of 2 tasks complete
@jasnell
jasnell approved these changes May 4, 2017
refack added a commit to refack/node that referenced this pull request May 6, 2017
test: fix flakyness with `yes.exe`
PR-URL: nodejs#12821
Fixes: nodejs#12817
Refs: nodejs#12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@refack refack closed this May 6, 2017

@refack refack force-pushed the refack:fix-12817 branch from 3035267 to 68c933c May 6, 2017

@refack

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

landed in 68c933c

@refack refack deleted the refack:fix-12817 branch May 6, 2017

@refack

This comment has been minimized.

Copy link
Member Author

commented May 6, 2017

Post land CI: https://ci.nodejs.org/job/node-test-commit/9691/

[update]
CI passed

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: fix flakyness with `yes.exe`
PR-URL: nodejs#12821
Fixes: nodejs#12817
Refs: nodejs#12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: fix flakyness with `yes.exe`
PR-URL: #12821
Fixes: #12817
Refs: #12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@refack I've landed this on v6.x, seems low risk, and should hopefully reduce CI flakyness on v6.x.

If you think it shouldn't have landed LMK.

@refack

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

If you think it shouldn't have landed LMK.

Should be Ok.

MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: fix flakyness with `yes.exe`
PR-URL: #12821
Fixes: #12817
Refs: #12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.