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.

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

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

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 PRs that do not need to wait for 48 hours to land. label Jan 22, 2018
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 22, 2018
@apapirovski
Copy link
Member Author

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

@apapirovski
Copy link
Member Author

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

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

@guybedford guybedford left a comment

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
Copy link
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 review January 22, 2018 17:23

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

@apapirovski
Copy link
Member Author

@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 commented Jan 22, 2018 •
edited
Loading

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

#18295 should cover the test cases here too.

@apapirovski
Copy link
Member Author

apapirovski commented Jan 22, 2018 •
edited
Loading

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

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

@guybedford guybedford left a comment

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

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

@guybedford
Copy link
Contributor

👍 sounds good to me.

@apapirovski
Copy link
Member Author

Landing...

@apapirovski
Copy link
Member Author

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>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
@MylesBorins
Copy link
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eugeneo eugeneo eugeneo approved these changes

@guybedford guybedford guybedford approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues 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