Skip to content

Fix failing tests on VSCode 1.74.0#1852

Merged
aeisenberg merged 2 commits intomainfrom
koesie10/fix-vscode-1.74.0
Dec 8, 2022
Merged

Fix failing tests on VSCode 1.74.0#1852
aeisenberg merged 2 commits intomainfrom
koesie10/fix-vscode-1.74.0

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Dec 8, 2022

This fixes the tests on VSCode 1.74.0. The issue is as follows:

  1. Jest wil try reset all mocks after each test, as it should.
  2. When Jest does this, it will loop over all global variables and check if they are mocks.
  3. One of the global variables it checks is _VSCODE_NODE_MODULES, which is a proxy object.
  4. When Jest checks whether it is a proxy by getting _isMockFunction on it, the get function on the proxy object will be called.
  5. This will in turn call require, which will try to load the non-existing _isMockFunction module. This throws the error we are seeing.

By removing the _VSCODE_NODE_MODULES property from the global object in the Jest environment, Jest will not try to reset it, and the tests should work again.

See: https://github.com/facebook/jest/blob/41bf2300895a2c00d0525d21260f0a392819871f/packages/jest-runtime/src/index.ts#L1173-L1186
See: https://github.com/microsoft/vscode/blob/ed442a9e99ff68a3bb9e4953081305766862f450/src/bootstrap-amd.js#L15

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This fixes the tests on VSCode 1.74.0. The issue is as follows:

1. Jest wil try reset all mocks after each test, as it should.
2. When Jest does this, it will loop over all global variables and check
if they are mocks.
3. One of the global variables it checks is _VSCODE_NODE_MODULES, which
is a proxy object.
4. When Jest checks whether it is a proxy by getting _isMockFunction on
it, the `get` function on the proxy object will be called.
5. This will in turn call require, which will try to load
the non-existing `_isMockFunction` module. This throws the error we are
seeing.

By removing the `_VSCODE_NODE_MODULES` property from the global object
in the Jest environment, Jest will not try to reset it, and the tests
should work again.

See: https://github.com/facebook/jest/blob/41bf2300895a2c00d0525d21260f0a392819871f/packages/jest-runtime/src/index.ts#L1173-L1186
See: https://github.com/microsoft/vscode/blob/ed442a9e99ff68a3bb9e4953081305766862f450/src/bootstrap-amd.js#L15
@koesie10 koesie10 added the secexp label Dec 8, 2022
@koesie10 koesie10 requested a review from a team December 8, 2022 11:20
@koesie10 koesie10 marked this pull request as ready for review December 8, 2022 11:20
@koesie10 koesie10 requested a review from a team as a code owner December 8, 2022 11:20
+ // The _VSCODE_NODE_MODULES is a proxy which will require a module if any property
+ // on it is accessed. This is a workaround for the fact that jest will call
+ // _isMockFunction on the module, which will cause that function to be required.
+ delete this.global._VSCODE_NODE_MODULES;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, this deletes a global variable. Is this variable used anywhere else?

Would it be safer to change the part of jest that checks for proxies in globals? We can explicitly avoid checking this variable. Eg-

            const globalMock = envGlobal[key];
            if (key !== '_VSCODE_NODE_MODULES' &&
              ((typeof globalMock === 'object' && globalMock !== null) ||
                typeof globalMock === 'function') &&
              globalMock._isMockFunction === true
            ) {
              globalMock.mockClear();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right that this deletes a global variable. It shouldn't be used anywhere else, it's only used when VSCode is loading code and the Jest environment should always load after all extensions have loaded. There seem to be some specific usecases where it is being used, but as long as we don't use any of those code paths while testing the extension, it should be fine.

I tried using this.global = {...this.global}; before deleting it from this.global, but this unfortunately doesn't work.

The reason for adding it in here rather than as a patch for the jest-runtime package is that I'd highly prefer to have as few patched packages as possible and this is something very specific to VSCode, so should be in the jest-runner-vscode. If we do patch jest-runtime, we could try to be more generic by checking for a Proxy object instead of checking for this specific key, that should be more generic and have the same effect.

Another option would be to wrap the global. _VSCODE_NODE_MODULES in another Proxy layer which specifically excludes _isMockFunction from being required. I believe this may actually be the safest option, and it would be something that can be done in jest-runner-vscode. I'll try whether this actually works and commit it if that does work.

Instead of deleting the complete `_VSCODE_NODE_MODULES` object, we now
use a `Proxy` to intercept the `_isMockFunction` property. This is safer
and will not delete a global variable that VSCode expects to exist.
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. This makes sense and I think the second approach is slightly better.

@aeisenberg
Copy link
Copy Markdown
Contributor

I am going to merge this PR right now since we have tests on semmle-code that are currently failing and should pass with this change in.

@aeisenberg aeisenberg merged commit 5138831 into main Dec 8, 2022
@aeisenberg aeisenberg deleted the koesie10/fix-vscode-1.74.0 branch December 8, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants