test: simplify knownGlobals logic

[cjihrig]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[lpinca]

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

[cjihrig]

CI failure seemed unrelated. One more try: https://ci.nodejs.org/job/node-test-pull-request/14838/

[BridgeAR]

After looking into the globals closer I am -1 on this.

These values are actually never tested for because these properties are all non-enumerable and for...in does not list does properties.

Instead, I recommend to remove the known globals check. It never worked properly so far and we never had an issue with globals as far as I can tell.

It is just a minor overhead in each test and a maintenance burden.

[cjihrig]

I think the question of whether or not we should do global checking is outside the scope of this issue. This is a small cleanup of the existing code. Would you please unblock this PR, and open an issue for further discussion.

[BridgeAR]

@cjihrig having these values in the known globals means they will be excluded from testing even though that should not be the case. So they should actually be removed. I opened #20717 because I looked a bit closer at the knownGlobals.

I agree that it is out of scope if the mechanism as such should be removed but just moving them around does not improve anything.

If you would like to keep this PR I suggest to just remove the values and I update my PR accodingly.

[cjihrig]

I think I misunderstood your original statement. I see what you mean now about the enumerability of those keys. I think we should go one of two ways:

1. Remove the non-enumerable keys from the global checks, as Ruben suggests above.
2. Make the check more thorough by checking everything on the global object. The known globals list would get bigger, and we'd have to use more than a for...in loop when inspecting the global object.

Thoughts from @nodejs/testing and others?

[Trott]

I'd prefer more thorough tests even at the expense of making them a hair slower, so I'm in favor of option 2, but I'm good with option 1 as well, as it isn't worse than what we have now, IIUC.

[apapirovski]

I'm fine with either direction. We could just use Object.getOwnPropertyNames instead of the for..in.

FWIW we also test not just for leaked globals but whether the globals have been accidentally replaced with an unexpected value.

[BridgeAR]

My take on it is to first remove the non-enumerable entries from known globals and to also remove the flag to skip the test as I did in #20717. I definitely think if we test for globals one way or the other, we should also test for the non-enumerable ones, so we should use Object.getOwnPropertyNames. To distinguish them from the enumerable ones we have to use a new list anyway, so going forward from #20717 should be pretty straight forward.
Then we just still have to decide if it would be best to remove the knownGlobals check from running with every test case and to replace it with one test that that loads all modules upfront (even though that would miss cases where globals are added due to some code execution) or not. I tend to the first as this test was added 8 years ago when there were no code reviews and it turned out that this test was misunderstood for quite a while (just recently enumerable entries were added to the list in a PR). IMHO the current check is just not useful. Our code reviews work quite well in this case, especially, since I can not remember that anyone tried to add globals in tests since I work on Node.js besides if they were meant to be added.

[Trott]

since I can not remember that anyone tried to add globals in tests since I work on Node.js besides if they were meant to be added.

Couldn't that be taken as a sign that the check was doing its job? (I've certainly had it fire on me a few times when I do some funky stuff locally while debugging.)

[Trott]

Couldn't that be taken as a sign that the check was doing its job? (I've certainly had it fire on me a few times when I do some funky stuff locally while debugging.)

To clarify: Maybe no one has ever tried to erroneously add a global in a PR because when they run such a test locally, it fails because of this check we're talking about right now.

[jasnell]

Where are we at on this one?

[cjihrig]

I'm working to update the global check to include all properties, including symbols. There are some edge cases, like NaN, but I haven't forgotten about this.