FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

aix, test: prolong debugged child process by jBarz · Pull Request #15774 · nodejs/node · GitHub

/ node Public

aix, test: prolong debugged child process - #15774

Closed
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:master.aix
ConversationCommits (1)ChecksFiles changed
Closed

aix, test: prolong debugged child process#15774
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:master.aix

Conversation

jBarz commented Oct 4, 2017 •
edited by gibfahn
Loading

Copy link
Copy Markdown
Contributor

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 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 mentioned this pull request Oct 10, 2017
Closed

gibfahn commented Oct 10, 2017

Copy link
Copy Markdown
Member

Needs a stress test

refack commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

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

gibfahn commented Oct 10, 2017

Copy link
Copy Markdown
Member

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 commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

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 commented Oct 13, 2017

Copy link
Copy Markdown
Contributor Author

Can we do the stress test again?

gibfahn commented Oct 13, 2017

Copy link
Copy Markdown
Member

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

gibfahn commented Oct 19, 2017

Copy link
Copy Markdown
Member

@jBarz if you rebase I'll stress test.

jBarz commented Oct 19, 2017

Copy link
Copy Markdown
Contributor Author

Thanks @gibfahn , done!

gibfahn commented Oct 19, 2017

Copy link
Copy Markdown
Member

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

gibfahn commented Oct 20, 2017

Copy link
Copy Markdown
Member

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

mhdawson approved these changes Oct 24, 2017
View reviewed changes

mhdawson left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

jBarz commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

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

refack approved these changes Oct 24, 2017
View reviewed changes

refack left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should stress test, but code LGTM

mhdawson commented Nov 2, 2017

Copy link
Copy Markdown
Member

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 commented Nov 2, 2017

Copy link
Copy Markdown
Member

arm failures were infra issues not related to this PR.

mhdawson commented Nov 2, 2017

Copy link
Copy Markdown
Member

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
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 <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson commented Nov 2, 2017

Copy link
Copy Markdown
Member

Landed as f1f0eb2

mhdawson closed this Nov 2, 2017
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
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 <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
cjihrig mentioned this pull request Nov 6, 2017
Merged
jackhorton mentioned this pull request Nov 8, 2017
Closed
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
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 <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn mentioned this pull request Nov 21, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
gireeshpunathil gireeshpunathil approved these changes
+3 more reviewers
refack refack approved these changes
mhdawson mhdawson approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL