8000 aix, test: prolong debugged child process by jBarz · Pull Request #15774 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    aix, test: prolong debugged child process #15774

    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
    Closed
    jBarz wants to merge 1 commit into from
    Closed

    aix, test: prolong debugged child process #15774

    jBarz 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 <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
    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 <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 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 <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 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

    @jasnell jasnell jasnell approved these changes

    @gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

    @mhdawson mhdawson mhdawson approved these changes

    +2 more reviewers

    @refack refack refack approved these changes

    @gibfahn 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

    @jBarz @gibfahn @refack @mhdawson @jasnell @gireeshpunathil @MylesBorins @nodejs-github-bot
    0