Skip to content

Conversation

@eugeneo
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

This change only touches inspector test.

Description of change

Test was updated to wait till the inspector processes socket closure.

CC: @ofrobots

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Aug 8, 2016
Copy link
Member

Choose a reason for hiding this comment

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

This looked okay to me? reinterpret_cast<…>() is an argument to uv_is_active, not EXPECT_EQ

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Right. I reverted the change.

@addaleaxaddaleax added the inspector Issues and PRs related to the V8 inspector protocol label Aug 8, 2016
@ofrobots
Copy link
Contributor

nit: since this is fixing a test, the commit message should probably use the 'test: ' prefix. Otherwise LGTM.

CI: https://ci.nodejs.org/job/node-test-pull-request/3582/. I have verified locally that this fixes #8006.

/cc @bnoordhuis as well.

@eugeneo
Copy link
ContributorAuthor

Thank you for the review. I updated the commit message.

@bnoordhuis
Copy link
Member

LGTM but can you update the commit log status line to e.g. test: fix failing inspector cctest?

I admit I don't understand why the test passes for me locally with and without this change.

Test was updated to wait till the inspector processes socket closure. Fixes: #8006
@eugeneo
Copy link
ContributorAuthor

@bnoordhuis I updated the message.

This test failure was intermittent - it looks like sockets closing on Mac is somehow more "async" then on other systems (or simply slower) so this test failed because the cleanup code saw inspector socket as still open. If I added a delay (e.g. printed a message to stderr) the test succeeded.

ofrobots pushed a commit that referenced this pull request Aug 10, 2016
Test was updated to wait till the inspector processes socket closure. Fixes: #8006 PR-URL: #8019 Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@ofrobots
Copy link
Contributor

Landed as 49e473a.

@cjihrigcjihrig mentioned this pull request Aug 10, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Test was updated to wait till the inspector processes socket closure. Fixes: #8006 PR-URL: #8019 Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@cjihrigcjihrig mentioned this pull request Aug 11, 2016
@eugeneoeugeneo deleted the failing_test branch August 16, 2016 00:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocoltestIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@eugeneo@ofrobots@bnoordhuis@addaleax@MylesBorins@nodejs-github-bot