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

Es6 module global leakage test #16341

Closed
wants to merge 6 commits into from
Closed

Es6 module global leakage test #16341

wants to merge 6 commits into from

Conversation

tinybeachthor
Copy link
Contributor

@tinybeachthor tinybeachthor commented Oct 20, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Oct 20, 2017
Fishrock123
Fishrock123 approved these changes Oct 20, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Seems like a good idea to me. CI: https://ci.nodejs.org/job/node-test-pull-request/10873/

test/addons/hello-world-esm/test.mjs Outdated
@@ -1,5 +1,4 @@
/* eslint-disable required-modules */

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please remove the whitespace changes? :D

@Fishrock123
Copy link
Contributor

(Sidenote: this will need to be squashed on landing, I think.)

Trott
Trott approved these changes Oct 22, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes

@Trott
Copy link
Member

Trott commented Oct 22, 2017

(CI failures are unrelated.)

Trott
Trott reviewed Oct 22, 2017
View reviewed changes
test/es-module/test-esm-require-cache.mjs Outdated
@@ -3,5 +3,6 @@ import '../common';
import '../fixtures/es-module-require-cache/preload.js';
import '../fixtures/es-module-require-cache/counter.js';
import assert from 'assert';

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Unrelated whitespace change?

mhdawson
mhdawson approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

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

@apapirovski
Copy link
Member

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

@BridgeAR
Copy link
Member

And another CI https://ci.nodejs.org/job/node-test-pull-request/11658/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017
@addaleax
Copy link
Member

Landed in 8075d82, thanks for the PR! 🎉

@addaleax addaleax closed this Nov 28, 2017
addaleax pushed a commit that referenced this pull request Nov 28, 2017
@tinybeachthor @addaleax
test: add es6 module global leakage tests
8075d82 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@tinybeachthor @MylesBorins
test: add es6 module global leakage tests
4210e04 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@tinybeachthor @MylesBorins
test: add es6 module global leakage tests
703cdd0 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@tinybeachthor @MylesBorins
test: add es6 module global leakage tests
36e2643 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@tinybeachthor @gibfahn
test: add es6 module global leakage tests
97888e0 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@tinybeachthor @gibfahn
test: add es6 module global leakage tests
ecb1f17 
* created testing es6 module for global leakage tests
* fixed a couple basic errors (vars -> lets, ...)
* added globals leakage detection to es module tests
* reversed whitespace change
* whitespace change reversed

PR-URL: #16341
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@apapirovski apapirovski apapirovski approved these changes

@jasnell jasnell jasnell approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.