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: properly tag anonymous namespaces #18583

Closed
wants to merge 1 commit into from
Closed

test: properly tag anonymous namespaces #18583

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Feb 5, 2018

For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

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

test

@mhdawson
test: properly tag anonymous namespaces
2418225 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.
@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Feb 5, 2018
@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

FYI @bnoordhuis

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018
bnoordhuis
bnoordhuis approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. FWIW, I mentioned it because the linter complained about it at one time. Maybe we lost that when we upgraded cpplint.

@mhdawson
Copy link
Member Author

mhdawson commented Feb 8, 2018

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

@mhdawson
Copy link
Member Author

mhdawson commented Feb 8, 2018

Opened #18663 for issue seen on ubuntu 1204

The failure on 1604 looks like a problem writing out the output, but here is another test run for the linux platforms to see if it recreates: https://ci.nodejs.org/job/node-test-commit-linux/16248/

@mhdawson
Copy link
Member Author

mhdawson commented Feb 8, 2018

No recreate on ubuntu1604. Full green CI on linux so landing.

@mhdawson
Copy link
Member Author

mhdawson commented Feb 8, 2018

Landed as fe442f6

@mhdawson mhdawson closed this Feb 8, 2018
mhdawson added a commit that referenced this pull request Feb 8, 2018
@mhdawson
test: properly tag anonymous namespaces
fe442f6 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@mhdawson @MylesBorins
test: properly tag anonymous namespaces
85a4cec 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@mhdawson @MylesBorins
test: properly tag anonymous namespaces
a03eeb2 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@mhdawson @MylesBorins
test: properly tag anonymous namespaces
a822814 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@mhdawson @MylesBorins
test: properly tag anonymous namespaces
2089814 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@mhdawson @MayaLekova
test: properly tag anonymous namespaces
fbd1329 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: nodejs#18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 25, 2018
@mhdawson @MylesBorins
test: properly tag anonymous namespaces
2780733 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@mhdawson @rvagg
test: properly tag anonymous namespaces
ad862a0 
For tests that use anonymous namespaces, some tagged the close
of the namespace with 'namespace' while others used
'anonymous namespace'. It was suggested I should use
'anonymous namespace' in a recent PR review so make all of the
tests consistent with this.

PR-URL: #18583
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mhdawson mhdawson deleted the anonymous-scope branch September 30, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@eugeneo eugeneo eugeneo approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. inspector Issues and PRs related to the V8 inspector protocol node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mhdawson @bnoordhuis @eugeneo @cjihrig @BridgeAR @MylesBorins @nodejs-github-bot