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

test: be explicit about polluting of global #6017

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 2, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

test crypto domain

Description of change

There was a comment in test-domain-crypto.js indicating that the
pollution of the global object with a domain property was
intentional. Provide more information in the comment so someone may
easily determine why. Use global.domain rather than declaring domain
without the var keyword to more clearly signal that the pollution is
intentional.

There was a comment in `test-domain-crypto.js` indicating that the
pollution of the `global` object with a `domain` property was
intentional. Provide more information in the comment so someone may
easily determine why. Use `global.domain` rather than declaring `domain`
without the `var` keyword to more clearly signal that the pollution is
intentional.
@Trott Trott added crypto Issues and PRs related to the crypto subsystem. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests. lts-watch-v4.x labels Apr 2, 2016
@bnoordhuis
Copy link
Member

Can you add a 'use strict' at the top? LGTM apart from that.

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

LGTM with the nit addressed

@cjihrig
Copy link
Contributor

cjihrig commented Apr 2, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

jasnell pushed a commit that referenced this pull request Apr 4, 2016
There was a comment in `test-domain-crypto.js` indicating that the
pollution of the `global` object with a `domain` property was
intentional. Provide more information in the comment so someone may
easily determine why. Use `global.domain` rather than declaring `domain`
without the `var` keyword to more clearly signal that the pollution is
intentional.

PR-URL: #6017
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Landed in ae2be27

@jasnell jasnell closed this Apr 4, 2016
Trott added a commit to Trott/io.js that referenced this pull request Apr 4, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

Refs: nodejs#6017
MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
There was a comment in `test-domain-crypto.js` indicating that the
pollution of the `global` object with a `domain` property was
intentional. Provide more information in the comment so someone may
easily determine why. Use `global.domain` rather than declaring `domain`
without the `var` keyword to more clearly signal that the pollution is
intentional.

PR-URL: #6017
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Apr 5, 2016
Trott added a commit to Trott/io.js that referenced this pull request Apr 6, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: nodejs#6047
Refs: nodejs#6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
There was a comment in `test-domain-crypto.js` indicating that the
pollution of the `global` object with a `domain` property was
intentional. Provide more information in the comment so someone may
easily determine why. Use `global.domain` rather than declaring `domain`
without the `var` keyword to more clearly signal that the pollution is
intentional.

PR-URL: #6017
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: #6047
Refs: #6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: #6047
Refs: #6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: #6047
Refs: #6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 17, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: #6047
Refs: #6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
The last change to this test landed before a nit about strict mode was
addressed, so this change addresses that.

PR-URL: #6047
Refs: #6017
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
@Trott Trott deleted the doctor branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants