add support for Explicit Resource Management to mocked functions by phryneas · Pull Request #14895 · jestjs/jest

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

    "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

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

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

Uh oh!

Conversation

phryneas commented Feb 11, 2024 • edited by SimenB Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

linux-foundation-easycla bot commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for jestjs ready!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas commented Feb 12, 2024

Uh oh!

mrazauskas commented Feb 12, 2024

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phryneas commented Feb 12, 2024

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phryneas commented Feb 11, 2024 •

edited by SimenB

Loading

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

edited

Loading

netlify bot commented Feb 11, 2024 •

edited

Loading