★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18293Nouvelle recherche | Portail wanayoo
Skip to content
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.

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

test: fix issues in inspector-helper #18293

Closed
wants to merge 2 commits into from
Closed

test: fix issues in inspector-helper #18293

wants to merge 2 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 22, 2018

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

@apapirovski apapirovski added the fast-track label Jan 22, 2018
@apapirovski apapirovski requested a review from guybedford Jan 22, 2018
@nodejs-github-bot nodejs-github-bot added the test label Jan 22, 2018
@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

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

@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

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

guybedford
guybedford previously approved these changes Jan 22, 2018
View changes
Copy link
Contributor

@guybedford guybedford left a comment

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

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 22, 2018

@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 review Jan 22, 2018

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

@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

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

@guybedford guybedford mentioned this pull request Jan 22, 2018
Closed
@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018 •
edited

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

@apapirovski apapirovski changed the title test: fix lint issues in inspector-helper test: fix issues in inspector-helper Jan 22, 2018
@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 22, 2018

#18295 should cover the test cases here too.

@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018 •
edited

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

guybedford
guybedford approved these changes Jan 22, 2018
View changes
Copy link
Contributor

@guybedford guybedford left a comment

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

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 22, 2018

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
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

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

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 22, 2018

👍 sounds good to me.

@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

Landing...

@apapirovski
Copy link
Member Author

@apapirovski apapirovski commented Jan 22, 2018

Landed in a3555d0

@apapirovski apapirovski deleted the fix-linter-18194 branch Jan 22, 2018
apapirovski added a commit that referenced this issue 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 issue 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 issue 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 issue 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>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Mar 20, 2018

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 issue 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eugeneo eugeneo

@guybedford guybedford

@lpinca lpinca

Assignees
No one assigned
Labels
fast-track test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@apapirovski @guybedford @MylesBorins @eugeneo @lpinca @nodejs-github-bot