test: compute list of expected globals from ESLint config file by aduh95 · Pull Request #42056 · nodejs/node

aduh95 · 2022-02-19T19:04:02Z

We don't want to rely on mutable globals for core modules. Instead of
maintaining a separate list of known globals in our test files, parse
the ESLint config to ensure all globals are restricted in the lib/
directory.

All globals must be in the allow list. Previously, only enumerable globals were verified.
This now to verify globals using their name rather than their value. This avoids triggering warnings for globals that are getters with an experimental or deprecation warning (e.g. fetch). It also solves the case where there are two global variables referencing the same object (e.g. global and globalThis).
For convenience and backward compatibility, it's still possible to pass the value of the global rather than its name so current tests can still pass.

@targos This may have consequences for V8 updates (if the new V8 version ships with new globals).

/cc @benjamingr

Blocked on ~~#42049~~ nodejs/build#2879.

Trott · 2022-02-19T22:00:34Z

test/common/index.js

-  atob,
-  btoa
-} = require('buffer');
+const parseEslintConfigForGlobals = require('./parseEslintConfigForGlobals');


Does this need to be a separate file? Should it be inlined so that people don't think it's something they can/should include themselves in their own tests?

RaisinTen

We need to synchronously read and parse lib/.eslintrc.yaml line-by-line for this to work correctly.

test/common/parseEslintConfigForGlobals.js

nodejs-github-bot · 2022-02-22T19:40:31Z

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

nodejs-github-bot · 2022-02-26T11:44:49Z

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

nodejs-github-bot · 2022-02-26T16:08:04Z

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

richardlau · 2022-02-26T16:30:28Z

Makefile

+
 .PHONY: jstest
-jstest: build-addons build-js-native-api-tests build-node-api-tests ## Runs addon tests and JS tests
+jstest: build-addons build-js-native-api-tests build-node-api-tests test/common/knownGlobals.json ## Runs addon tests and JS tests


I think this is going to break jstest (and targets such as test-only) for builds from the source tar ball as we strip out anything related to linting (e.g. lib/.eslintrc.yaml) from those: #5618

node/Makefile

Line 1167 in aa97c9d

find $(TARNAME)/ -name ".eslint*" -maxdepth 2 | xargs $(RM)

richardlau · 2022-02-26T17:13:47Z

Makefile

 	ssh $(STAGINGSERVER) "touch nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/$(TARNAME).pkg.done"

-$(TARBALL): release-only doc-only
+$(TARBALL): release-only doc-only test/common/knownGlobals.json


This target will also probably need to copy in the generated test/common/knownGlobals.json as the tarball is prepared in $(TARNAME)/ from a fresh git checkout-index.

addressed

nodejs-github-bot · 2022-02-27T14:32:27Z

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

outdated

nodejs-github-bot · 2022-02-27T15:20:42Z

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

nodejs-github-bot · 2022-02-27T23:02:09Z

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

nodejs-github-bot · 2022-03-01T00:10:42Z

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

Refs: nodejs/node#42056

aduh95 · 2022-03-02T10:36:26Z

Blocked on nodejs/build#2879

targos · 2022-03-02T10:40:26Z

What's the reason for using Python to generate the file?

aduh95 · 2022-03-02T10:42:57Z

What's the reason for using Python to generate the file?

Because the tarball action doesn't have any available node.

targos · 2022-03-02T10:46:03Z

Don't GitHub runners all have node available by default?

aduh95 · 2022-03-02T11:04:12Z

Don't GitHub runners all have node available by default?

Maybe it was too old to work? I don't exactly remember tbh. Using Python as also the upside of being already integrated in the test suite. Is there a reason not to use Python?

targos · 2022-03-02T11:07:10Z

Is there a reason not to use Python?

I hope that one day, Python won't be required anymore to work on node, so adding things that rely on it doesn't help.
I'm not blocking this though, just wanted to understand how you came to using it.

aduh95 · 2022-03-02T11:16:24Z

OK gotcha! Hopefully this won't be too much of a burden to port this to whatever replacement we find – it could be a sed script if it wasn't for Windows support.
Do you have an opinion on what this PR is trying to do?

targos · 2022-03-02T12:10:51Z

test/parallel/test-repl-autocomplete.js

 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

+common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);


Maybe add common.allowReplGlobals() or similar to avoid repetition?

targos · 2022-03-02T12:11:48Z

I'm ok with the bullet points in your OP, and the changes to test/ LGTM. I don't feel comfortable reviewing the other changes.

RaisinTen · 2022-03-02T15:16:39Z

Is there a reason not to use Python?

If we avoid Python and generate the JSON in JS, it would be convenient to do it without creating any additional files which would address #42056 (comment) but I don't know how much time it would add overall when we run all the tests.

aduh95 added the blocked PRs that are blocked by other issues or PRs. label Feb 19, 2022

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Feb 19, 2022

Trott reviewed Feb 19, 2022

View reviewed changes

RaisinTen previously requested changes Feb 20, 2022

View reviewed changes

test/common/parseEslintConfigForGlobals.js Outdated Show resolved Hide resolved

aduh95 force-pushed the tie-known-globals-with-eslint-config branch from b3964c2 to d6e4f2d Compare February 21, 2022 11:34

aduh95 marked this pull request as draft February 21, 2022 15:20

aduh95 force-pushed the tie-known-globals-with-eslint-config branch from d6e4f2d to 96115e9 Compare February 22, 2022 19:32

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 22, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2022

aduh95 added 2 commits February 26, 2022 12:33

test: compute list of expected globals from ESLint config file

fbb89f2

write json file from python

9a5ec5f

aduh95 force-pushed the tie-known-globals-with-eslint-config branch from 96115e9 to 9a5ec5f Compare February 26, 2022 11:44

fixup! write json file from python

48e859d

aduh95 marked this pull request as ready for review February 26, 2022 16:07

aduh95 added 3 commits February 26, 2022 17:17

fixup! fixup! write json file from python

afdec40

lint fix

5f9d376

fixup! lint fix

76b5491

richardlau previously requested changes Feb 26, 2022

View reviewed changes

fix for tarball

f1cb9bf

richardlau reviewed Feb 26, 2022

View reviewed changes

aduh95 added 3 commits February 27, 2022 13:38

move to python

ef7dd44

copy generated file in the tarball

8d54abb

fix lint

d85d9b8

aduh95 added 2 commits February 27, 2022 16:01

don't swallow errors when we don't mean to

0446006

fix for \r support

c11b45d

aduh95 marked this pull request as draft February 28, 2022 00:46

aduh95 added 2 commits March 1, 2022 01:02

fix for tarball

642a1ad

create JSON file before testing addongs on Windows

945c835

aduh95 added a commit to nodejs/build that referenced this pull request Mar 1, 2022

jenkins: fix Windows Debug test

23392f7

Refs: nodejs/node#42056

aduh95 mentioned this pull request Mar 1, 2022

jenkins: fix Windows Debug test nodejs/build#2879

Open

aduh95 marked this pull request as ready for review March 1, 2022 00:46

respect test-root option

9f4470d

aduh95 added the blocked PRs that are blocked by other issues or PRs. label Mar 2, 2022

targos reviewed Mar 2, 2022

View reviewed changes

jasnell approved these changes Mar 7, 2022

View reviewed changes

This was referenced Mar 8, 2022

CI Reliability 2022-03-08 nodejs/reliability#214

Open

CI Reliability 2022-03-09 nodejs/reliability#215

Open

aduh95 mentioned this pull request May 21, 2022

undici leaking Symbol to global space in 18.2 (but not 18.1) #43157

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

aduh95 mentioned this pull request Sep 13, 2024

test: merge knownGlobals in common #54915

Closed

Uh oh!

Conversation

aduh95 commented Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 22, 2022

Uh oh!

nodejs-github-bot commented Feb 26, 2022

Uh oh!

nodejs-github-bot commented Feb 26, 2022

Uh oh!

richardlau Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

richardlau Feb 26, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 27, 2022

Uh oh!

nodejs-github-bot commented Feb 27, 2022

Uh oh!

nodejs-github-bot commented Feb 27, 2022

Uh oh!

nodejs-github-bot commented Mar 1, 2022

Uh oh!

aduh95 commented Mar 2, 2022

Uh oh!

targos commented Mar 2, 2022

Uh oh!

aduh95 commented Mar 2, 2022

Uh oh!

targos commented Mar 2, 2022

Uh oh!

aduh95 commented Mar 2, 2022

Uh oh!

targos commented Mar 2, 2022

Uh oh!

aduh95 commented Mar 2, 2022

Uh oh!

targos Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

targos commented Mar 2, 2022

Uh oh!

RaisinTen commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduh95 commented Feb 19, 2022 •

edited

Loading