-
Notifications
You must be signed in to change notification settings - Fork 30k
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
repl,test: fix global check #20717
repl,test: fix global check #20717
Conversation
These values are all non-enumerable and will never be checked. By removing them, we make sure they will not become enumerable unnoticed.
This aligns these globals with the regular context.
This flag is partially used in tests where it was not necessary and it is always possible to replace this flag with `common.allowGlobals`. This makes sure all globals are truly tested for.
why is @nodejs/tsc tagged as an owner here=? |
This is done automatically. I think this is still somewhat WIP. |
The change to Line 82 in 19374fd
|
@nodejs/repl @nodejs/testing |
It would be nice if this could get some reviews :-) |
It would be nice to get another LG before landing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
These values are all non-enumerable and will never be checked. By removing them, we make sure they will not become enumerable unnoticed. PR-URL: nodejs#20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This aligns these globals with the regular context. PR-URL: nodejs#20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This flag is partially used in tests where it was not necessary and it is always possible to replace this flag with `common.allowGlobals`. This makes sure all globals are truly tested for. PR-URL: nodejs#20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Landed in 8254e38...becc3ec |
These values are all non-enumerable and will never be checked. By removing them, we make sure they will not become enumerable unnoticed. PR-URL: #20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This aligns these globals with the regular context. PR-URL: #20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This flag is partially used in tests where it was not necessary and it is always possible to replace this flag with `common.allowGlobals`. This makes sure all globals are truly tested for. PR-URL: #20717 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
test: remove untested knownGlobals
:repl: make console, module and require non-enumerable
:test: remove common.globalCheck
:This fixes repl inconsistencies and removes code to actually make the globals check stricter.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes