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

add support for Explicit Resource Management to mocked functions #14895

Merged
merged 15 commits into from
Feb 20, 2024

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Feb 11, 2024

Summary

This fixes #14294

Spied-on functions can now be instantiated with using instead of const and they will be restored once the current scope is left.

Test plan

I added a test to jest-mock an integration test.

Copy link

linux-foundation-easycla bot commented Feb 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Feb 11, 2024

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit 6c9f0e6
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65d470979769800008716bad
😎 Deploy Preview https://deploy-preview-14895--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

package.json Outdated Show resolved Hide resolved
if (!Symbol.dispose) {
Object.defineProperty(Symbol, 'dispose', {
get() {
return Symbol.for('nodejs.dispose');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit weird, and the result of #14888 - usually, it should be possible to polyfill Symbol.dispose in any possible way, e.g. by doing Symbol.dispose ??= Symbol.for('dispose'), but because of #14888 it needs to explicitly be polyfilled with Symbol.for('nodejs.dispose').

I'm not sure if that change over there was a good idea, especially because that polyfill doesn't seem to be available in all scopes (that's why we need to polyfill here in the first place).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that also explains the failing node16 tests.

Copy link
Member

Choose a reason for hiding this comment

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

that's what the symbol is in a nodejs app, tho. so why would it be an issue?

But if we instead move the test to an e2e test as mentioned elsewhere, then we could just skip that tests on node versions which don't support it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bit irritating to me first because jest picks up Symbol.dispose as Symbol.for('nodejs.dispose'), while in the test itself was undefined, so it needed to be polyfilled to the same value (while in a normal application, you can polyfill it as pretty much anything and don't need to match the "outside runtime").

But I guess it makes sense that jest itself might see something different than the JestEnvironment itself.

Comment on lines 2457 to 2463
using mockedLog = jest.spyOn(console, 'log');
expect(jest.isMockFunction(console.log)).toBeTruthy();

console.log('test');
expect(mockedLog).toHaveBeenCalled();
}
expect(jest.isMockFunction(console.log)).toBeFalsy();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a common usage:

using mockedLog = jest.spyOn(console, 'log'); is only mocked inside of the current scope, and as soon as the block is left, the mock is restored.

Copy link
Member

Choose a reason for hiding this comment

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

this is awesome!

@phryneas
Copy link
Contributor Author

Generally, I don't really know what to make of these node16 failures, so I'd be happy for any feedback. As this feature is not available there, is it sensible to skip the tests there?

@mrazauskas
Copy link
Contributor

As this feature is not available there, is it sensible to skip the tests there?

Yes. Usually tests are skipped, if some feature is not supported. The onNodeVersions() helper is used for that (look around for usage examples):

export function onNodeVersions(
versionRange: string,
testBody: () => void,

It would be good to mention the feature in documentation. That is a good place to clear out the requirements like Node.js or TypeScript versions.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

super exciting stuff!

can you add a changelog entry as well?

tsconfig.json Outdated
@@ -8,6 +8,7 @@
"declaration": true,
"emitDeclarationOnly": true,
"stripInternal": true,
"lib": ["es2021", "ESNext.Disposable"],
Copy link
Member

Choose a reason for hiding this comment

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

do we need this with the inline <reference lib="ESNext.Disposable" />?

packages/jest-mock/src/index.ts Show resolved Hide resolved
Comment on lines 2457 to 2463
using mockedLog = jest.spyOn(console, 'log');
expect(jest.isMockFunction(console.log)).toBeTruthy();

console.log('test');
expect(mockedLog).toHaveBeenCalled();
}
expect(jest.isMockFunction(console.log)).toBeFalsy();
Copy link
Member

Choose a reason for hiding this comment

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

this is awesome!

Copy link
Contributor Author

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

I moved the tests into an integration test - I think code-wise it's as clean as I can get it at this point.
I'll take another look at the docs.

packages/jest-mock/src/index.ts Show resolved Hide resolved
@phryneas
Copy link
Contributor Author

I've also added some docs. I hope the place works, and if you have any suggestions for better examples, I'm very open to feedback. Good docs examples are not my biggest strength :)

@phryneas phryneas requested a review from SimenB February 12, 2024 21:43
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

we're missing a file to actually run the new E2E test - but I'll fix that up myself 🙂

docs/JestObjectAPI.md Outdated Show resolved Hide resolved
docs/JestObjectAPI.md Outdated Show resolved Hide resolved
docs/JestObjectAPI.md Outdated Show resolved Hide resolved
e2e/explicit-resource-management/__tests__/index.js Outdated Show resolved Hide resolved
e2e/explicit-resource-management/__tests__/index.js Outdated Show resolved Hide resolved
@@ -155,9 +155,9 @@ export default class NodeEnvironment implements JestEnvironment<Timer> {
if ('asyncDispose' in Symbol && !('asyncDispose' in global.Symbol)) {
const globalSymbol = global.Symbol as unknown as SymbolConstructor;
// @ts-expect-error - it's readonly - but we have checked above that it's not there
globalSymbol.asyncDispose = globalSymbol('nodejs.asyncDispose');
globalSymbol.asyncDispose = globalSymbol.for('nodejs.asyncDispose');
Copy link
Member

Choose a reason for hiding this comment

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

oopsie 😅

@SimenB SimenB merged commit 63db50f into jestjs:main Feb 20, 2024
72 of 73 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add support for 'using' keyword to dispose mocks/spies
3 participants