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

Test: Make spies reactive so that they can be logged by addon-actions #26740

Merged
merged 13 commits into from
Apr 10, 2024
38 changes: 14 additions & 24 deletions code/addons/actions/src/loaders.ts
Original file line number Diff line number Diff line change
@@ -1,36 +1,26 @@
/* eslint-disable no-underscore-dangle */
import type { LoaderFunction } from '@storybook/types';
import { global } from '@storybook/global';
import type { onMockCall as onMockCallType } from '@storybook/test';
import { action } from './runtime';

export const tinySpyInternalState = Symbol.for('tinyspy:spy');
let subscribed = false;

const attachActionsToFunctionMocks: LoaderFunction = (context) => {
const logActionsWhenMockCalled: LoaderFunction = (context) => {
const {
args,
parameters: { actions },
} = context;
if (actions?.disable) return;

Object.entries(args)
.filter(
([, value]) =>
typeof value === 'function' && '_isMockFunction' in value && value._isMockFunction
)
.forEach(([key, value]) => {
// See this discussion for context:
// https://github.com/vitest-dev/vitest/pull/5352
const previous =
value.getMockImplementation() ??
(tinySpyInternalState in value ? value[tinySpyInternalState]?.getOriginal() : undefined);
if (previous?._actionAttached !== true && previous?.isAction !== true) {
const implementation = (...params: unknown[]) => {
action(key)(...params);
return previous?.(...params);
};
implementation._actionAttached = true;
value.mockImplementation(implementation);
}
});
if (
!subscribed &&
'__STORYBOOK_TEST_ON_MOCK_CALL__' in global &&
typeof global.__STORYBOOK_TEST_ON_MOCK_CALL__ === 'function'
) {
const onMockCall = global.__STORYBOOK_TEST_ON_MOCK_CALL__ as typeof onMockCallType;
onMockCall((mock, args) => action(mock.getMockName())(args));
subscribed = true;
}
};

export const loaders: LoaderFunction[] = [attachActionsToFunctionMocks];
export const loaders: LoaderFunction[] = [logActionsWhenMockCalled];
1 change: 1 addition & 0 deletions code/lib/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"util": "^0.12.4"
},
"devDependencies": {
"tinyspy": "^2.2.0",
"ts-dedent": "^2.2.0",
"type-fest": "~2.19",
"typescript": "^5.3.2"
Expand Down
4 changes: 3 additions & 1 deletion code/lib/test/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { type LoaderFunction } from '@storybook/csf';
import chai from 'chai';
import { global } from '@storybook/global';
import { expect as rawExpect } from './expect';
import { clearAllMocks, resetAllMocks, restoreAllMocks } from './spy';
import { clearAllMocks, onMockCall, resetAllMocks, restoreAllMocks } from './spy';

export * from './spy';

Expand Down Expand Up @@ -39,3 +39,5 @@ const resetAllMocksLoader: LoaderFunction = ({ parameters }) => {
// We are using this as a default Storybook loader, when the test package is used. This avoids the need for optional peer dependency workarounds.
// eslint-disable-next-line no-underscore-dangle
(global as any).__STORYBOOK_TEST_LOADERS__ = [resetAllMocksLoader];
// eslint-disable-next-line no-underscore-dangle
(global as any).__STORYBOOK_TEST_ON_MOCK_CALL__ = onMockCall;
15 changes: 15 additions & 0 deletions code/lib/test/src/spy.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { it, vi, expect, beforeEach } from 'vitest';
import { fn, onMockCall } from './spy';

const vitestSpy = vi.fn();

beforeEach(() => {
const unsubscribe = onMockCall(vitestSpy);
return () => unsubscribe();
});

it('mocks are reactive', () => {
const storybookSpy = fn();
storybookSpy(1);
expect(vitestSpy).toHaveBeenCalledWith(storybookSpy, [1]);
});
52 changes: 49 additions & 3 deletions code/lib/test/src/spy.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,63 @@
/* eslint-disable @typescript-eslint/no-shadow */
import type { Mock, MockInstance } from '@vitest/spy';
import {
spyOn,
spyOn as vitestSpyOn,
isMockFunction,
fn,
fn as vitestFn,
mocks,
type MaybeMocked,
type MaybeMockedDeep,
type MaybePartiallyMocked,
type MaybePartiallyMockedDeep,
} from '@vitest/spy';
import type { SpyInternalImpl } from 'tinyspy';
import * as tinyspy from 'tinyspy';

export type * from '@vitest/spy';

export { spyOn, isMockFunction, fn, mocks };
export { isMockFunction, mocks };

type Listener = (mock: MockInstance, args: unknown[]) => void;
let listeners: Listener[] = [];

export function onMockCall(callback: Listener): () => void {
listeners = [...listeners, callback];
return () => {
listeners = listeners.filter((listener) => listener !== callback);
};
}
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved

// @ts-expect-error TS is hard you know
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
export const spyOn: typeof vitestSpyOn = (...args) => {
const mock = vitestSpyOn(...(args as Parameters<typeof vitestSpyOn>));
return reactiveMock(mock);
};

export function fn<TArgs extends any[] = any, R = any>(): Mock<TArgs, R>;
export function fn<TArgs extends any[] = any[], R = any>(
implementation: (...args: TArgs) => R
): Mock<TArgs, R>;
export function fn<TArgs extends any[] = any[], R = any>(implementation?: (...args: TArgs) => R) {
const mock = implementation ? vitestFn(implementation) : vitestFn();
return reactiveMock(mock);
}

function reactiveMock(mock: MockInstance) {
const reactive = listenWhenCalled(mock);
const originalMockImplementation = reactive.mockImplementation.bind(null);

Choose a reason for hiding this comment

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

Could you please check use case for spying on methods without redefining them #28419

It was working in Storybook 7, but not in Storybook 8

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added a fix for this 8.2, could you check that @dmy-leanix?

Choose a reason for hiding this comment

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

@kasperpeulen Currently can't check in my repo as in 8.2 some paths were changed and during running storybook I have:

Cannot find module 'storybook/internal/common'

Looks like related to this: 3a1e61c

Most probably nx or angular has own usages and this change wasn't backward compatible to it. And I can't update nx and angular right now

Copy link
Member

Choose a reason for hiding this comment

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

@dmy-leanix I'd need a repro to investigate.

I suspect you may have multiple versions of the storybook package.

Choose a reason for hiding this comment

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

@kasperpeulen "No dependencies found matching storybook", do I need storybook additionally to @storybook/*?

Choose a reason for hiding this comment

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

@kasperpeulen yes, after adding storybook dependency it works fine, didn't know that this dependency should also be add

Copy link
Member

Choose a reason for hiding this comment

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

Storybook for angular (as well as a lot of other packages) has a peerDependency on storybook starting 8.2.

"storybook": "workspace:^",

The storybook package has been a required devDependency since 7.0.

It should have been added by this auto-migration:

export const sbBinary: Fix<SbBinaryRunOptions> = {
id: 'storybook-binary',
versionRange: ['<7', '>=7'],
async check({ packageManager, storybookVersion }) {
const packageJson = await packageManager.retrievePackageJson();
const nrwlStorybookVersion = await packageManager.getPackageVersion('@nrwl/storybook');
const sbBinaryVersion = await packageManager.getPackageVersion('sb');
const storybookBinaryVersion = await packageManager.getPackageVersion('storybook');
// Nx provides their own binary, so we don't need to do anything
if (nrwlStorybookVersion) {
return null;
}
const hasSbBinary = !!sbBinaryVersion;
const hasStorybookBinary = !!storybookBinaryVersion;
if (!hasSbBinary && hasStorybookBinary) {
return null;
}
return {
hasSbBinary,
hasStorybookBinary,
storybookVersion,
packageJson,
};
},
prompt({ storybookVersion, hasSbBinary, hasStorybookBinary }) {
const sbFormatted = chalk.cyan(`Storybook ${storybookVersion}`);
const storybookBinaryMessage = !hasStorybookBinary
? `We've detected you are using ${sbFormatted} without Storybook's ${chalk.magenta(
'storybook'
)} binary. Starting in Storybook 7.0, it has to be installed.`
: '';
const extraMessage = hasSbBinary
? "You're using the 'sb' binary and it should be replaced, as 'storybook' is the recommended way to run Storybook.\n"
: '';
return dedent`
${storybookBinaryMessage}
${extraMessage}
More info: ${chalk.yellow(
'https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#start-storybook--build-storybook-binaries-removed'
)}
`;
},
async run({
result: { packageJson, hasSbBinary, hasStorybookBinary },
packageManager,
dryRun,
skipInstall,
}) {
if (hasSbBinary) {
logger.info(`✅ Removing 'sb' dependency`);
if (!dryRun) {
await packageManager.removeDependencies(
{ skipInstall: skipInstall || !hasStorybookBinary, packageJson },
['sb']
);
}
}
if (!hasStorybookBinary) {
logger.log();
logger.info(`✅ Adding 'storybook' as dev dependency`);
logger.log();
if (!dryRun) {
const versionToInstall = getStorybookVersionSpecifier(packageJson);
await packageManager.addDependencies(
{ installAsDevDependencies: true, packageJson, skipInstall },
[`storybook@${versionToInstall}`]
);
}
}
},
};

...when you upgraded from 6.x to 7.x.

Choose a reason for hiding this comment

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

@ndelangen thank you! yes, strange I didn't get error related to this before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndelangen This automigration didn't run for Nx users. @yannbf fixed that here:
#28601

reactive.mockImplementation = (fn) => listenWhenCalled(originalMockImplementation(fn));
return reactive;
}

function listenWhenCalled(mock: MockInstance) {
const state = tinyspy.getInternalState(mock as unknown as SpyInternalImpl);
const impl = state.impl?.bind(null);
state.willCall((...args) => {
listeners.forEach((listener) => listener(mock, args));
return impl?.(...args);
});
return mock;
}

/**
* Calls [`.mockClear()`](https://vitest.dev/api/mock#mockclear) on every mocked function. This will only empty `.mock` state, it will not reset implementation.
Expand Down
1 change: 1 addition & 0 deletions code/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -6667,6 +6667,7 @@ __metadata:
"@vitest/expect": "npm:1.3.1"
"@vitest/spy": "npm:^1.3.1"
chai: "npm:^4.4.1"
tinyspy: "npm:^2.2.0"
ts-dedent: "npm:^2.2.0"
type-fest: "npm:~2.19"
typescript: "npm:^5.3.2"
Expand Down