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: compute list of expected globals from ESLint config file #42056

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 19, 2022

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.

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Feb 19, 2022
@nodejs-github-bot 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
atob,
btoa
} = require('buffer');
const parseEslintConfigForGlobals = require('./parseEslintConfigForGlobals');
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

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

test/common/parseEslintConfigForGlobals.js Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the tie-known-globals-with-eslint-config branch from b3964c2 to d6e4f2d Compare February 21, 2022 11:34
@aduh95 aduh95 marked this pull request as draft February 21, 2022 15:20
@aduh95 aduh95 force-pushed the tie-known-globals-with-eslint-config branch from d6e4f2d to 96115e9 Compare February 22, 2022 19:32
@aduh95 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 force-pushed the tie-known-globals-with-eslint-config branch from 96115e9 to 9a5ec5f Compare February 26, 2022 11:44
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 marked this pull request as ready for review February 26, 2022 16:07
@nodejs-github-bot
Copy link
Collaborator

Makefile Outdated
.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
Copy link
Member

Choose a reason for hiding this comment

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

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)

Makefile Outdated
@@ -1140,7 +1140,7 @@ pkg-upload: pkg
scp -p $(TARNAME).pkg $(STAGINGSERVER):nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/$(TARNAME).pkg
ssh $(STAGINGSERVER) "touch nodejs/$(DISTTYPEDIR)/$(FULLVERSION)/$(TARNAME).pkg.done"

$(TARBALL): release-only doc-only
$(TARBALL): release-only doc-only test/common/knownGlobals.json
Copy link
Member

Choose a reason for hiding this comment

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

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.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 marked this pull request as draft February 28, 2022 00:46
@nodejs-github-bot
Copy link
Collaborator

aduh95 added a commit to nodejs/build that referenced this pull request Mar 1, 2022
@aduh95 aduh95 marked this pull request as ready for review March 1, 2022 00:46
@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Mar 2, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Mar 2, 2022

Blocked on nodejs/build#2879

@targos
Copy link
Member

targos commented Mar 2, 2022

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

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 2, 2022

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

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

@targos
Copy link
Member

targos commented Mar 2, 2022

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

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 2, 2022

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

targos commented Mar 2, 2022

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

aduh95 commented Mar 2, 2022

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?

@@ -15,6 +15,8 @@ common.skipIfDumbTerminal();
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

common.allowGlobals('require', '_', '_error', ...require('module').builtinModules);
Copy link
Member

Choose a reason for hiding this comment

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

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

@targos
Copy link
Member

targos commented Mar 2, 2022

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants