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

aix, test: prolong debugged child process #15774

Closed
wants to merge 1 commit into from
Closed

aix, test: prolong debugged child process #15774

wants to merge 1 commit into from

Conversation

jBarz
Copy link
Contributor

@jBarz jBarz commented Oct 4, 2017 •
edited by gibfahn
Loading

On AIX, there is a possibility that the child process is done before the parent debugger can connect to it. So prolong the child process by factor of 2.

Fixes: #14897

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)

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Oct 4, 2017
@jBarz jBarz mentioned this pull request Oct 10, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented Oct 10, 2017

Needs a stress test

@refack
Copy link
Contributor

refack commented Oct 10, 2017

https://ci.nodejs.org/job/node-stress-single-test/1446/nodes=aix61-ppc64/

@gibfahn
Copy link
Member

gibfahn commented Oct 10, 2017

Still looking a bit flaky: 1237 OK: 1228 NOT OK: 9 TOTAL: 9999

Is there a way for the child process to pause until the debugger is attached?

@refack
Copy link
Contributor

refack commented Oct 10, 2017

Is there a way for the child process to pause until the debugger is attached?

Maybe replace --inspect=0 with --inspect-brk=0 (there's already a Runtime.runIfWaitingForDebugger that will "continue" execution)

@jBarz
Copy link
Contributor Author

jBarz commented Oct 13, 2017

Can we do the stress test again?

@gibfahn
Copy link
Member

gibfahn commented Oct 13, 2017

@jBarz not till nodejs/build#775 (comment) is resolved.

@gibfahn
Copy link
Member

gibfahn commented Oct 19, 2017

@jBarz if you rebase I'll stress test.

@jBarz
Copy link
Contributor Author

jBarz commented Oct 19, 2017

Thanks @gibfahn , done!

@gibfahn
Copy link
Member

gibfahn commented Oct 19, 2017

Stress: https://ci.nodejs.org/job/node-stress-single-test/1457

@gibfahn
Copy link
Member

gibfahn commented Oct 20, 2017

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

mhdawson
mhdawson approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM the change but I think we also need to revert the AIX part of 7027191 so that it is no longer marked as flaky for AIX.

@jBarz
aix, test: pause child until parent is ready
34d5b9e 
Pause child on startup using inspect-brk=0 until the parent debugger
is ready.
@jBarz
Copy link
Contributor Author

jBarz commented Oct 24, 2017

I made another change which no longer marks this test as flaky for aix and windows.

refack
refack approved these changes Oct 24, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should stress test, but code LGTM

@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

I think earlier stress test showed it was good.

New CI since it has been 2 weeks: https://ci.nodejs.org/job/node-test-pull-request/11152/

@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

arm failures were infra issues not related to this PR.

@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

Windows failure was #16688

so net is that CI is good with respect to this change.

mhdawson pushed a commit that referenced this pull request Nov 2, 2017
@jBarz @mhdawson
test: pause child until parent is ready
f1f0eb2 
Pause child on startup using inspect-brk=0 until the parent debugger
is ready.

PR-URL: #15774
Fixes: #14897
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

Landed as f1f0eb2

@mhdawson mhdawson closed this Nov 2, 2017
@jBarz jBarz deleted the master.aix branch November 3, 2017 00:07
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@jBarz @cjihrig
test: pause child until parent is ready
5077faf 
Pause child on startup using inspect-brk=0 until the parent debugger
is ready.

PR-URL: nodejs#15774
Fixes: nodejs#14897
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
@jackhorton jackhorton mentioned this pull request Nov 8, 2017
Closed
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@jBarz @gibfahn
test: pause child until parent is ready
1e8a421 
Pause child on startup using inspect-brk=0 until the parent debugger
is ready.

PR-URL: #15774
Fixes: #14897
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jBarz @gibfahn @refack @mhdawson @jasnell @gireeshpunathil @MylesBorins @nodejs-github-bot