Skip to content

test: fix issues in inspector-helper - #18293

Closed
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:fix-linter-18194
Closed

test: fix issues in inspector-helper#18293
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:fix-linter-18194

Conversation

@apapirovski

@apapirovskiapapirovski commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

Fixes linting issues introduced in #18194

This needs to be fast-tracked.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@apapirovskiapapirovski added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 22, 2018
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 22, 2018
@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/110/

@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

I think parallel/test-inspector-esm is also broken... looking into it

guybedford
guybedford previously approved these changes Jan 22, 2018
View reviewed changes

@guybedfordguybedford left a comment

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.

Thanks for the quick fix on this, apologies for the noise.

@guybedford

guybedford commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

@apapirovski it seems I messed up a sync to an older commit... I can PR the fix or we can revert for now as well.

@guybedford
guybedford dismissed their stale reviewJanuary 22, 2018 17:23

I'm going to rather PR the fix with the test cases as well

@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

@guybedford I should have a fix shortly. I'll include in this PR.

@guybedfordguybedford mentioned this pull request Jan 22, 2018
Closed
@apapirovski

apapirovski commented Jan 22, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@apapirovskiapapirovski changed the title test: fix lint issues in inspector-helpertest: fix issues in inspector-helperJan 22, 2018
@guybedford

guybedford commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

#18295 should cover the test cases here too.

@apapirovski

apapirovski commented Jan 22, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Will land this when CI is done since it has approvals.

guybedford
guybedford approved these changes Jan 22, 2018
View reviewed changes

@guybedfordguybedford left a comment

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.

Test case fix looks good to me, pending CI checks.

@guybedford

guybedford commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

Seems like the Windows tests have unstable here. If you think that could have been introduced by this I'd be open to a revert.

@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

@guybedford I'm very certain those have been unstable for a while. I think we can land this.

@guybedford

guybedford commented Jan 22, 2018

Copy link
Copy Markdown
Contributor

👍 sounds good to me.

@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

Landing...

@apapirovski

apapirovski commented Jan 22, 2018

Copy link
Copy Markdown
ContributorAuthor

Landed in a3555d0

@apapirovski
apapirovski deleted the fix-linter-18194 branch January 22, 2018 18:27
apapirovski added a commit that referenced this pull request Jan 22, 2018
@apapirovski
test: fix a bug & lint issues in inspector-helper
a3555d0
PR-URL: #18293
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@apapirovski@MylesBorins
test: fix a bug & lint issues in inspector-helper
130d6f3
PR-URL: #18293
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski@MylesBorins
test: fix a bug & lint issues in inspector-helper
df34171
PR-URL: #18293
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@apapirovski@MylesBorins
test: fix a bug & lint issues in inspector-helper
19abee1
PR-URL: #18293
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski@MayaLekova
test: fix a bug & lint issues in inspector-helper
5ec8994
PR-URL: nodejs#18293
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedfordguybedfordguybedford approved these changes
@lpincalpincalpinca approved these changes
+1 more reviewer
@eugeneoeugeneoeugeneo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@apapirovski@guybedford@MylesBorins@eugeneo@lpinca@nodejs-github-bot