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.

Sign up for GitHub

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
Merged

add support for Explicit Resource Management to mocked functions #14895

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f1ea1f7
add support for Explicit Resource Management to mocked functions
phryneas Feb 10, 2024
7cfc9d0
review feedback
phryneas Feb 10, 2024
a10c151
move to integration test
phryneas Feb 10, 2024
e5cf3d2
remove global babel plugin
phryneas Feb 10, 2024
82c82b1
remove accidental line
phryneas Feb 10, 2024
fe13eee
add some docs
phryneas Feb 10, 2024
b117fad
changelog
phryneas Feb 10, 2024
b8e9f02
prettier
phryneas Feb 10, 2024
46031c1
Update CHANGELOG.md
SimenB Feb 20, 2024
37401b9
Apply suggestions from code review
SimenB Feb 20, 2024
9ebd286
Merge branch 'main' into pr/jest-using
SimenB Feb 20, 2024
ce99d2d
run new tests
SimenB Feb 20, 2024
832523f
Update e2e/explicit-resource-management/__tests__/index.js
SimenB Feb 20, 2024
da7d505
bah
SimenB Feb 20, 2024
6c9f0e6
skip test on old nodes
SimenB Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ module.exports = {
plugins: [
['@babel/plugin-transform-modules-commonjs', {allowTopLevelThis: true}],
require.resolve('./scripts/babel-plugin-jest-require-outside-vm'),
'@babel/plugin-proposal-explicit-resource-management',
],
presets: [
[
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"version": "0.0.0",
"devDependencies": {
"@babel/core": "^7.11.6",
"@babel/plugin-proposal-explicit-resource-management": "^7.23.9",
"@babel/plugin-transform-modules-commonjs": "^7.1.0",
"@babel/preset-env": "^7.1.0",
"@babel/preset-react": "^7.12.1",
Expand Down Expand Up @@ -81,7 +82,7 @@
"tempy": "^1.0.0",
"ts-node": "^10.5.0",
"tstyche": "^1.0.0-beta.9",
"typescript": "^5.0.4",
phryneas marked this conversation as resolved.
Show resolved Hide resolved
"typescript": "^5.2.2",
"webpack": "^5.68.0",
"webpack-node-externals": "^3.0.0",
"which": "^4.0.0"
Expand Down
31 changes: 31 additions & 0 deletions packages/jest-mock/src/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ import * as util from 'util';
import {type Context, createContext, runInContext, runInNewContext} from 'vm';
import {ModuleMocker, fn, mocked, spyOn} from '../';

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.

},
});
}

describe('moduleMocker', () => {
let moduleMocker: ModuleMocker;
let mockContext: Context;
Expand Down Expand Up @@ -2432,3 +2440,26 @@ test('`fn` and `spyOn` do not throw', () => {
spyOn({apple: () => {}}, 'apple');
}).not.toThrow();
});

describe('Explicit Resource Management', () => {
it('jest.fn state should be restored with the `using` keyword', () => {
const mock = jest.fn();
{
using inScope = mock.mockReturnValue(2);
expect(inScope()).toBe(2);
expect(mock()).toBe(2);
}
expect(mock()).not.toBe(2);
});

it('should be restored with the `using` keyword', () => {
{
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!

});
});
8 changes: 7 additions & 1 deletion packages/jest-mock/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
* LICENSE file in the root directory of this source tree.
*/

/// <reference lib="ESNext.Disposable" />
SimenB marked this conversation as resolved.
Show resolved Hide resolved

/* eslint-disable local/ban-types-eventually, local/prefer-rest-params-eventually */

import {isPromise} from 'jest-util';
Expand Down Expand Up @@ -131,7 +133,8 @@ type ResolveType<T extends FunctionLike> =
type RejectType<T extends FunctionLike> =
ReturnType<T> extends PromiseLike<any> ? unknown : never;

export interface MockInstance<T extends FunctionLike = UnknownFunction> {
export interface MockInstance<T extends FunctionLike = UnknownFunction>
extends Disposable {
_isMockFunction: true;
_protoImpl: Function;
getMockImplementation(): T | undefined;
Expand Down Expand Up @@ -797,6 +800,9 @@ export class ModuleMocker {
};

f.withImplementation = withImplementation.bind(this);
if (Symbol.dispose) {
f[Symbol.dispose] = f.mockRestore;
}

function withImplementation(fn: T, callback: () => void): void;
function withImplementation(
Expand Down
1 change: 1 addition & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" />?


"strict": true,

Expand Down
30 changes: 27 additions & 3 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,18 @@ __metadata:
languageName: node
linkType: hard

"@babel/plugin-proposal-explicit-resource-management@npm:^7.23.9":
version: 7.23.9
resolution: "@babel/plugin-proposal-explicit-resource-management@npm:7.23.9"
dependencies:
"@babel/helper-plugin-utils": ^7.22.5
"@babel/plugin-syntax-explicit-resource-management": ^7.23.3
peerDependencies:
"@babel/core": ^7.0.0-0
checksum: d7a37ea28178e251fe289895cf4a37fee47195122a3e172eb088be9b0a55d16d2b2ac3cd6569e9f94c9f9a7744a812f3eba50ec64e3d8f7a48a4e2b0f2caa959
languageName: node
linkType: hard

"@babel/plugin-proposal-export-default-from@npm:^7.0.0":
version: 7.23.3
resolution: "@babel/plugin-proposal-export-default-from@npm:7.23.3"
Expand Down Expand Up @@ -841,6 +853,17 @@ __metadata:
languageName: node
linkType: hard

"@babel/plugin-syntax-explicit-resource-management@npm:^7.23.3":
version: 7.23.3
resolution: "@babel/plugin-syntax-explicit-resource-management@npm:7.23.3"
dependencies:
"@babel/helper-plugin-utils": ^7.22.5
peerDependencies:
"@babel/core": ^7.0.0-0
checksum: 60306808e4680b180a2945d13d4edc7aba91bbd43b300271b89ebd3d3d0bc60f97c6eb7eaa7b9e2f7b61bb0111c24469846f636766517da5385351957c264eb9
languageName: node
linkType: hard

"@babel/plugin-syntax-export-default-from@npm:^7.0.0, @babel/plugin-syntax-export-default-from@npm:^7.23.3":
version: 7.23.3
resolution: "@babel/plugin-syntax-export-default-from@npm:7.23.3"
Expand Down Expand Up @@ -3000,6 +3023,7 @@ __metadata:
resolution: "@jest/monorepo@workspace:."
dependencies:
"@babel/core": ^7.11.6
"@babel/plugin-proposal-explicit-resource-management": ^7.23.9
"@babel/plugin-transform-modules-commonjs": ^7.1.0
"@babel/preset-env": ^7.1.0
"@babel/preset-react": ^7.12.1
Expand Down Expand Up @@ -3077,7 +3101,7 @@ __metadata:
tempy: ^1.0.0
ts-node: ^10.5.0
tstyche: ^1.0.0-beta.9
typescript: ^5.0.4
typescript: ^5.2.2
webpack: ^5.68.0
webpack-node-externals: ^3.0.0
which: ^4.0.0
Expand Down Expand Up @@ -20334,7 +20358,7 @@ __metadata:
languageName: node
linkType: hard

"typescript@npm:5.3.3, typescript@npm:^5.0.4":
"typescript@npm:5.3.3, typescript@npm:^5.0.4, typescript@npm:^5.2.2":
version: 5.3.3
resolution: "typescript@npm:5.3.3"
bin:
Expand All @@ -20344,7 +20368,7 @@ __metadata:
languageName: node
linkType: hard

"typescript@patch:[email protected]#~builtin<compat/typescript>, typescript@patch:typescript@^5.0.4#~builtin<compat/typescript>":
"typescript@patch:[email protected]#~builtin<compat/typescript>, typescript@patch:typescript@^5.0.4#~builtin<compat/typescript>, typescript@patch:typescript@^5.2.2#~builtin<compat/typescript>":
version: 5.3.3
resolution: "typescript@patch:typescript@npm%3A5.3.3#~builtin<compat/typescript>::version=5.3.3&hash=e012d7"
bin:
Expand Down
Loading