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

Addon A11y: Fix skipped status handling in Testing Module #30077

Merged
merged 7 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions code/addons/test/src/components/TestProviderRender.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ const baseState: TestProviderState<Details, Config> = {
coverage: false,
},
details: {
config: {
a11y: false,
coverage: false,
},
testResults: [
{
endTime: 0,
Expand Down Expand Up @@ -141,6 +145,10 @@ export const WithCoverageNegative: Story = {
...config,
...baseState,
details: {
config: {
a11y: false,
coverage: true,
},
testResults: [],
coverageSummary: {
percentage: 20,
Expand All @@ -162,6 +170,10 @@ export const WithCoverageWarning: Story = {
...baseState,
details: {
testResults: [],
config: {
a11y: false,
coverage: true,
},
coverageSummary: {
percentage: 50,
status: 'warning',
Expand All @@ -182,6 +194,10 @@ export const WithCoveragePositive: Story = {
...baseState,
details: {
testResults: [],
config: {
a11y: false,
coverage: true,
},
coverageSummary: {
percentage: 80,
status: 'positive',
Expand All @@ -206,6 +222,10 @@ export const Editing: Story = {
},
details: {
testResults: [],
config: {
a11y: false,
coverage: false,
},
},
},
},
Expand All @@ -229,6 +249,10 @@ export const EditingAndWatching: Story = {
},
details: {
testResults: [],
config: {
a11y: true,
coverage: true, // should be automatically disabled in the UI
},
},
},
},
Expand Down
29 changes: 22 additions & 7 deletions code/addons/test/src/components/TestProviderRender.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,14 @@ export const TestProviderRender: FC<
return 'unknown';
}

if (!a11yResults) {
const definedA11yResults = a11yResults?.filter(Boolean) ?? [];

if (!definedA11yResults || definedA11yResults.length === 0) {
valentinpalkovic marked this conversation as resolved.
Show resolved Hide resolved
return 'unknown';
}

const failed = a11yResults.some((result) => result?.status === 'failed');
const warning = a11yResults.some((result) => result?.status === 'warning');
const failed = definedA11yResults.some((result) => result?.status === 'failed');
const warning = definedA11yResults.some((result) => result?.status === 'warning');
valentinpalkovic marked this conversation as resolved.
Show resolved Hide resolved

if (failed) {
return 'negative';
Expand All @@ -154,11 +156,24 @@ export const TestProviderRender: FC<
return 'positive';
}, [state.running, isA11yAddon, config.a11y, a11yResults]);

const a11yNotPassedAmount = a11yResults?.filter(
(result) => result?.status === 'failed' || result?.status === 'warning'
).length;
const a11yNotPassedAmount = state.config?.a11y
? a11yResults?.filter((result) => result?.status === 'failed' || result?.status === 'warning')
.length
: undefined;

const a11ySkippedAmount =
state.running || !state?.details.config?.a11y || !state.config.a11y
? null
: a11yResults?.filter((result) => !result).length;

const a11ySkippedLabel = a11ySkippedAmount
? a11ySkippedAmount === 1 && isStoryEntry
? '(skipped)'
: `(${a11ySkippedAmount} skipped)`
: '';

const storyId = isStoryEntry ? entryId : undefined;

const results = (state.details?.testResults || [])
.flatMap((test) => {
if (!entryId) {
Expand Down Expand Up @@ -333,7 +348,7 @@ export const TestProviderRender: FC<
)}
{isA11yAddon && (
<ListItem
title={<ItemTitle enabled={config.a11y}>Accessibility</ItemTitle>}
title={<ItemTitle enabled={config.a11y}>Accessibility {a11ySkippedLabel}</ItemTitle>}
onClick={
(a11yStatus === 'negative' || a11yStatus === 'warning') && a11yResults.length
? () => {
Expand Down
1 change: 1 addition & 0 deletions code/addons/test/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface Config {

export type Details = {
testResults: TestResult[];
config: Config;
coverageSummary?: {
status: 'positive' | 'warning' | 'negative' | 'unknown';
percentage: number;
Expand Down
1 change: 1 addition & 0 deletions code/addons/test/src/node/reporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ export class StorybookReporter implements Reporter {
} as TestingModuleProgressReportProgress,
details: {
testResults,
config: this.testManager.config,
},
};
}
Expand Down
14 changes: 7 additions & 7 deletions code/addons/test/src/node/test-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,11 @@ describe('TestManager', () => {

it('should handle watch mode request', async () => {
const testManager = await TestManager.start(mockChannel, options);
expect(testManager.watchMode).toBe(false);
expect(testManager.config.watchMode).toBe(false);
expect(createVitest).toHaveBeenCalledTimes(1);

await testManager.handleWatchModeRequest({ providerId: TEST_PROVIDER_ID, watchMode: true });
expect(testManager.watchMode).toBe(true);
expect(testManager.config.watchMode).toBe(true);
expect(createVitest).toHaveBeenCalledTimes(1); // shouldn't restart vitest
});

Expand Down Expand Up @@ -149,37 +149,37 @@ describe('TestManager', () => {

it('should handle coverage toggling', async () => {
const testManager = await TestManager.start(mockChannel, options);
expect(testManager.coverage).toBe(false);
expect(testManager.config.coverage).toBe(false);
expect(createVitest).toHaveBeenCalledTimes(1);
createVitest.mockClear();

await testManager.handleConfigChange({
providerId: TEST_PROVIDER_ID,
config: { coverage: true, a11y: false },
});
expect(testManager.coverage).toBe(true);
expect(testManager.config.coverage).toBe(true);
expect(createVitest).toHaveBeenCalledTimes(1);
createVitest.mockClear();

await testManager.handleConfigChange({
providerId: TEST_PROVIDER_ID,
config: { coverage: false, a11y: false },
});
expect(testManager.coverage).toBe(false);
expect(testManager.config.coverage).toBe(false);
expect(createVitest).toHaveBeenCalledTimes(1);
});

it('should temporarily disable coverage on focused tests', async () => {
vitest.globTestSpecs.mockImplementation(() => tests);
const testManager = await TestManager.start(mockChannel, options);
expect(testManager.coverage).toBe(false);
expect(testManager.config.coverage).toBe(false);
expect(createVitest).toHaveBeenCalledTimes(1);

await testManager.handleConfigChange({
providerId: TEST_PROVIDER_ID,
config: { coverage: true, a11y: false },
});
expect(testManager.coverage).toBe(true);
expect(testManager.config.coverage).toBe(true);
expect(createVitest).toHaveBeenCalledTimes(2);

await testManager.handleRunRequest({
Expand Down
30 changes: 19 additions & 11 deletions code/addons/test/src/node/test-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ import { VitestManager } from './vitest-manager';
export class TestManager {
vitestManager: VitestManager;

watchMode = false;

coverage = false;
config = {
watchMode: false,
coverage: false,
a11y: false,
};

constructor(
private channel: Channel,
Expand All @@ -44,13 +46,19 @@ export class TestManager {
return;
}

const previousConfig = this.config;

this.config = {
...this.config,
...payload.config,
} satisfies Config;

process.env.VITEST_STORYBOOK_CONFIG = JSON.stringify(payload.config);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: storing the entire payload.config in env var may expose more configuration than intended - consider only storing necessary fields


if (this.coverage !== payload.config.coverage) {
this.coverage = payload.config.coverage;
if (previousConfig.coverage !== payload.config.coverage) {
try {
await this.vitestManager.restartVitest({
coverage: this.coverage,
coverage: this.config.coverage,
});
} catch (e) {
this.reportFatalError('Failed to change coverage configuration', e);
Expand All @@ -62,7 +70,7 @@ export class TestManager {
if (payload.providerId !== TEST_PROVIDER_ID) {
return;
}
this.watchMode = payload.watchMode;
this.config.watchMode = payload.watchMode;

if (payload.config) {
this.handleConfigChange({
Expand All @@ -71,14 +79,14 @@ export class TestManager {
});
}

if (this.coverage) {
if (this.config.coverage) {
try {
if (payload.watchMode) {
// if watch mode is toggled on and coverage is already enabled, restart vitest without coverage to automatically disable it
await this.vitestManager.restartVitest({ coverage: false });
} else {
// if watch mode is toggled off and coverage is already enabled, restart vitest with coverage to automatically re-enable it
await this.vitestManager.restartVitest({ coverage: this.coverage });
await this.vitestManager.restartVitest({ coverage: this.config.coverage });
}
} catch (e) {
this.reportFatalError('Failed to change watch mode while coverage was enabled', e);
Expand All @@ -104,7 +112,7 @@ export class TestManager {
as a coverage report for a subset of stories is not useful.
*/
const temporarilyDisableCoverage =
this.coverage && !this.watchMode && (payload.storyIds ?? []).length > 0;
this.config.coverage && !this.config.watchMode && (payload.storyIds ?? []).length > 0;
if (temporarilyDisableCoverage) {
await this.vitestManager.restartVitest({
coverage: false,
Expand All @@ -117,7 +125,7 @@ export class TestManager {

if (temporarilyDisableCoverage) {
// Re-enable coverage if it was temporarily disabled because of a subset of stories was run
await this.vitestManager.restartVitest({ coverage: this.coverage });
await this.vitestManager.restartVitest({ coverage: this.config.coverage });
}
} catch (e) {
this.reportFatalError('Failed to run tests', e);
Expand Down
4 changes: 2 additions & 2 deletions code/addons/test/src/node/vitest-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export class VitestManager {
this.filterStories(story, spec.moduleId, { include, exclude, skip })
);
if (matches.length) {
if (!this.testManager.watchMode) {
if (!this.testManager.config.watchMode) {
// Clear the file cache if watch mode is not enabled
this.updateLastChanged(spec.moduleId);
}
Expand Down Expand Up @@ -320,7 +320,7 @@ export class VitestManager {

// when watch mode is disabled, don't trigger any tests (below)
// but still invalidate the cache for the changed file, which is handled above
if (!this.testManager.watchMode) {
if (!this.testManager.config.watchMode) {
return;
}
await this.runAffectedTests(file);
Expand Down
Loading