add support for Explicit Resource Management to mocked functions #14895

phryneas · 2024-02-11T17:57:18Z

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.

linux-foundation-easycla · 2024-02-11T17:57:22Z

The committers listed above are authorized under a signed CLA.

✅ login: phryneas / name: Lenz Weber-Tronic (82c82b1, f1ea1f7, a10c151, 7cfc9d0, b8e9f02, fe13eee, e5cf3d2, b117fad)
✅ login: SimenB / name: Simen Bekkhus (9ebd286, ce99d2d, 46031c1, 832523f, da7d505, 37401b9, 6c9f0e6)

netlify · 2024-02-11T17:57:24Z

✅ 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... Use your smartphone camera to open QR code link.

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

package.json

phryneas · 2024-02-11T18:01:33Z

packages/jest-mock/src/__tests__/index.test.ts

+if (!Symbol.dispose) {
+  Object.defineProperty(Symbol, 'dispose', {
+    get() {
+      return Symbol.for('nodejs.dispose');


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).

I think that also explains the failing node16 tests.

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

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.

phryneas · 2024-02-11T18:02:35Z

packages/jest-mock/src/__tests__/index.test.ts

+      using mockedLog = jest.spyOn(console, 'log');
+      expect(jest.isMockFunction(console.log)).toBeTruthy();
+
+      console.log('test');
+      expect(mockedLog).toHaveBeenCalled();
+    }
+    expect(jest.isMockFunction(console.log)).toBeFalsy();


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.

this is awesome!

phryneas · 2024-02-12T10:25:01Z

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 · 2024-02-12T10:43:08Z

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):

jest/packages/test-utils/src/ConditionalTest.ts

Lines 33 to 35 in 8bbe2a3

    
           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.

SimenB

super exciting stuff!

can you add a changelog entry as well?

SimenB · 2024-02-12T10:41:44Z

tsconfig.json

@@ -8,6 +8,7 @@
    "declaration": true,
    "emitDeclarationOnly": true,
    "stripInternal": true,
+    "lib": ["es2021", "ESNext.Disposable"],


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

packages/jest-mock/src/index.ts

SimenB · 2024-02-12T10:44:24Z

packages/jest-mock/src/__tests__/index.test.ts

+      using mockedLog = jest.spyOn(console, 'log');
+      expect(jest.isMockFunction(console.log)).toBeTruthy();
+
+      console.log('test');
+      expect(mockedLog).toHaveBeenCalled();
+    }
+    expect(jest.isMockFunction(console.log)).toBeFalsy();


this is awesome!

phryneas

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

phryneas · 2024-02-12T21:43:42Z

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 :)

SimenB

thanks!

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

docs/JestObjectAPI.md

e2e/explicit-resource-management/__tests__/index.js

SimenB · 2024-02-20T09:16:43Z

packages/jest-environment-node/src/index.ts

@@ -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');


oopsie 😅

github-actions · 2024-03-23T00:08:18Z

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.

phryneas added 8 commits February 10, 2024 11:25

add support for Explicit Resource Management to mocked functions

f1ea1f7

review feedback

7cfc9d0

move to integration test

a10c151

remove global babel plugin

e5cf3d2

remove accidental line

82c82b1

add some docs

fe13eee

changelog

b117fad

prettier

b8e9f02

phryneas commented Feb 11, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

phryneas commented Feb 11, 2024

View reviewed changes

SimenB requested changes Feb 12, 2024

View reviewed changes

phryneas commented Feb 12, 2024

View reviewed changes

packages/jest-mock/src/index.ts Show resolved Hide resolved

phryneas requested a review from SimenB February 12, 2024 21:43

Update CHANGELOG.md

46031c1

SimenB approved these changes Feb 20, 2024

View reviewed changes

SimenB added 5 commits February 20, 2024 09:01

Apply suggestions from code review

37401b9

Merge branch 'main' into pr/jest-using

9ebd286

run new tests

ce99d2d

Update e2e/explicit-resource-management/__tests__/index.js

832523f

bah

da7d505

SimenB reviewed Feb 20, 2024

View reviewed changes

SimenB approved these changes Feb 20, 2024

View reviewed changes

skip test on old nodes

6c9f0e6

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

github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for Explicit Resource Management to mocked functions #14895

add support for Explicit Resource Management to mocked functions #14895

phryneas commented Feb 11, 2024 •

edited by SimenB

linux-foundation-easycla bot commented Feb 11, 2024 •

edited

netlify bot commented Feb 11, 2024 •

edited

phryneas Feb 11, 2024

phryneas Feb 11, 2024

SimenB Feb 12, 2024

phryneas Feb 12, 2024

phryneas Feb 11, 2024

SimenB Feb 12, 2024

phryneas commented Feb 12, 2024

mrazauskas commented Feb 12, 2024

SimenB left a comment

SimenB Feb 12, 2024

SimenB Feb 12, 2024

phryneas left a comment

phryneas commented Feb 12, 2024

SimenB left a comment

SimenB Feb 20, 2024

github-actions bot commented Mar 23, 2024

add support for Explicit Resource Management to mocked functions #14895

add support for Explicit Resource Management to mocked functions #14895

Conversation

phryneas commented Feb 11, 2024 • edited by SimenB

Summary

Test plan

linux-foundation-easycla bot commented Feb 11, 2024 • edited

netlify bot commented Feb 11, 2024 • edited

✅ Deploy Preview for jestjs ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Feb 12, 2024

mrazauskas commented Feb 12, 2024

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas left a comment

Choose a reason for hiding this comment

phryneas commented Feb 12, 2024

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2024

phryneas commented Feb 11, 2024 •

edited by SimenB

linux-foundation-easycla bot commented Feb 11, 2024 •

edited

netlify bot commented Feb 11, 2024 •

edited