8000 test: ensure callback runs in test-vm-sigint by Trott · Pull Request #7768 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    test: ensure callback runs in test-vm-sigint #7768

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

    test: ensure callback runs in test-vm-sigint #7768

    Trott wants to merge 1 commit into from

    Conversation

    @Trott
    Copy link
    Member
    @Trott Trott commented Jul 16, 2016
    Checklist
    • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
    • commit message follows commit guidelines
    Affected core subsystem(s)

    test vm

    Description of change

    Make sure the child process close callback runs exactly one time.

    /cc @addaleax

    @Trott Trott added vm Issues and PRs related to the vm subsystem. test Issues and PRs related to the tests. labels Jul 16, 2016
    @Trott
    Copy link
    Member Author
    Trott commented Jul 16, 2016

    CI: https://ci.nodejs.org/job/node-test-pull-request/3320/

    @addaleax
    Copy link
    Member
    addaleax commented Jul 16, 2016

    LGTM, thanks

    @mscdex
    Copy link
    Contributor
    mscdex commented Jul 16, 2016 •
    edited
    Loading

    Minor nit, but perhaps s/insure/ensure/ in the commit message?

    @addaleax
    Copy link
    Member
    addaleax commented Jul 16, 2016

    The test failed on FreeBSD with no change in symptoms: https://ci.nodejs.org/job/node-test-commit-freebsd/3322/nodes=freebsd10-64/tapTestReport/test.tap-1066/

    @cjihrig
    Copy link
    Contributor
    cjihrig commented Jul 16, 2016

    LGTM even if the test is not ultimately fixed.

    @Trott
    Copy link
    Member Author
    Trott commented Jul 19, 2016

    @mscdex s/insure/ensure/ done

    @mscdex mscdex changed the title test: insure callback runs in test-vm-sigint test: ensure callback runs in test-vm-sigint Jul 19, 2016
    @Trott
    Copy link
    Member Author
    Trott commented Jul 19, 2016

    @addaleax Still looks good to you even though it doesn't fix the test? (I still think it's a minor improvement but I don't want to make assumptions that anyone agrees with that.)

    @addaleax
    Copy link
    Member
    addaleax commented Jul 19, 2016

    Yep, I never expected this to fix the test in the first place. :)

    Trott added a commit to Trott/io.js that referenced this pull request Jul 20, 2016
    @Trott
    test: ensure callback runs in test-vm-sigint
    fd02c93 
    PR-URL: nodejs#7768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    @Trott
    Copy link
    Member Author
    Trott commented Jul 20, 2016

    Landed in fd02c93

    @Trott Trott closed this Jul 20, 2016
    evanlucas pushed a commit that referenced this pull request Jul 21, 2016
    @Trott @evanlucas
    test: ensure callback runs in test-vm-sigint
    61542e8 
    PR-URL: #7768
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    @evanlucas evanlucas mentioned this pull request Jul 21, 2016
    Merged
    @Trott Trott deleted the mustcall branch January 13, 2022 22:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    5 participants

    @Trott @addaleax @mscdex @cjihrig @MylesBorins
    0