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

Use configured downloadsFolder for Cypress #164

Merged
merged 10 commits into from
Jul 5, 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
7 changes: 7 additions & 0 deletions .changeset/slimy-cups-protect.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
andrewortwein marked this conversation as resolved.
Show resolved Hide resolved
'@chromatic-com/playwright': patch
'@chromatic-com/cypress': patch
---

- Use the configured `downloadsFolder` in Cypress as the output dir for archives
- Move Playwright-related path logic out of the shared package into the Playwright package
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ test-archives
**/playwright/.cache/
packages/playwright/test-results/
packages/cypress/tests/cypress/downloads/
packages/cypress/tests/cypress/test-downloads/
packages/*/coverage/
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
"scripts": {
"archive-storybook:playwright": "CHROMATIC_ARCHIVE_LOCATION=packages/playwright/test-results packages/playwright/dist/bin/archive-storybook.js",
"build-archive-storybook:playwright": "CHROMATIC_ARCHIVE_LOCATION=packages/playwright/test-results packages/playwright/dist/bin/build-archive-storybook.js",
"archive-storybook:cypress": "CHROMATIC_ARCHIVE_LOCATION=packages/cypress/tests/cypress/downloads packages/cypress/dist/bin/archive-storybook.js",
"build-archive-storybook:cypress": "CHROMATIC_ARCHIVE_LOCATION=packages/cypress/tests/cypress/downloads packages/cypress/dist/bin/build-archive-storybook.js",
"archive-storybook:cypress": "CHROMATIC_ARCHIVE_LOCATION=packages/cypress/tests/cypress/test-downloads packages/cypress/dist/bin/archive-storybook.js",
"build-archive-storybook:cypress": "CHROMATIC_ARCHIVE_LOCATION=packages/cypress/tests/cypress/test-downloads packages/cypress/dist/bin/build-archive-storybook.js",
"clean": "yarn workspaces foreach --all --parallel run clean",
"prebuild": "yarn clean",
"build": "yarn workspaces foreach --all --topological-dev --parallel run build",
Expand Down
6 changes: 3 additions & 3 deletions packages/cypress/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
chromaticStorybookParams: ChromaticStorybookParameters;
pageUrl: string;
viewport: Viewport;
outputDir: string;
}

interface WriteArchivesParams extends WriteParams {
Expand All @@ -34,6 +35,7 @@
chromaticStorybookParams,
pageUrl,
viewport,
outputDir,
}: WriteArchivesParams) => {
const bufferedArchiveList = Object.entries(resourceArchive).map(([key, value]) => {
return [
Expand All @@ -59,9 +61,7 @@
await writeTestResult(
{
titlePath: testTitlePath,
// this will store it at ./cypress/downloads (the last directory doesn't matter)
// TODO: change so we don't have to do this trickery
outputDir: './cypress/downloads/some',
outputDir,
pageUrl,
viewport,
},
Expand All @@ -87,7 +87,7 @@
}): Promise<null> => {
try {
const { webSocketDebuggerUrl } = await Version({
host,

Check warning on line 90 in packages/cypress/src/index.ts

View workflow job for this annotation

GitHub Actions / test / test

Unexpected console statement
port,
});

Expand Down
1 change: 1 addition & 0 deletions packages/cypress/src/support.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ afterEach(() => {
height: Cypress.config('viewportHeight'),
width: Cypress.config('viewportWidth'),
},
outputDir: Cypress.config('downloadsFolder'),
},
});
});
Expand Down
9 changes: 9 additions & 0 deletions packages/cypress/tests/cypress.config.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import { defineConfig } from 'cypress';
import { installPlugin } from '../dist';
import { existsSync } from 'node:fs';

export default defineConfig({
// `downloadsFolder` cannot be overridden in tests, so we're setting
// this to a non-default value for asserting in the tests
downloadsFolder: 'cypress/test-downloads',
skitterm marked this conversation as resolved.
Show resolved Hide resolved
// needed since we use common mock images between Cypress and Playwright
fixturesFolder: '../../../test-server/fixtures',
screenshotOnRunFailure: false,
e2e: {
baseUrl: 'http://localhost:3000',
setupNodeEvents(on, config) {
installPlugin(on, config);
on('task', {
directoryExists(directoryName) {
return existsSync(directoryName);
},
});
},
},
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Snapshots are disabled because we're asserting on file system concerns,
// not testing anything visual
it(
'downloads archives to the user-specified folder',
{ env: { disableAutoSnapshot: true } },
skitterm marked this conversation as resolved.
Show resolved Hide resolved
() => {
cy.visit('/asset-paths/query-params');
const chromaticArchivesDir = `${Cypress.config('downloadsFolder')}/chromatic-archives`;
cy.task('directoryExists', chromaticArchivesDir).then((dirExists) => {
skitterm marked this conversation as resolved.
Show resolved Hide resolved
expect(dirExists).to.be.true;
});
}
);
6 changes: 5 additions & 1 deletion packages/playwright/src/makeTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
trackRun,
DEFAULT_GLOBAL_RESOURCE_ARCHIVE_TIMEOUT_MS,
} from '@chromatic-com/shared-e2e';
import { join } from 'node:path';
tevanoff marked this conversation as resolved.
Show resolved Hide resolved
import { chromaticSnapshots, takeSnapshot } from './takeSnapshot';
import { createResourceArchive } from './createResourceArchive';

Expand Down Expand Up @@ -72,8 +73,11 @@ export const performChromaticSnapshot = async (
...(cropToViewport && { cropToViewport }),
};

// TestInfo.outputDir gives us the test-specific subfolder (https://playwright.dev/docs/api/class-testconfig#test-config-output-dir);
// we want to write one level above that
const outputDir = join(testInfo.outputDir, '..');
await writeTestResult(
{ ...testInfo, pageUrl: page.url(), viewport: page.viewportSize() },
{ ...testInfo, outputDir, pageUrl: page.url(), viewport: page.viewportSize() },
Object.fromEntries(snapshots),
resourceArchive,
chromaticStorybookParams
Expand Down
16 changes: 8 additions & 8 deletions packages/shared/src/write-archive/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('writeTestResult', () => {
// the default output directory in playwright
{
titlePath: ['file.spec.ts', 'Test Story'],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand Down Expand Up @@ -90,7 +90,7 @@ describe('writeTestResult', () => {
// the default output directory in playwright
{
titlePath: ['file.spec.ts', 'Toy Story'],
outputDir: resolve('test-results/toy-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('writeTestResult', () => {
{
titlePath: ['file.spec.ts', 'Test Story'],
// simulates setting a custom output directory in Playwright
outputDir: resolve('some-custom-directory/directory/test-story-chromium'),
outputDir: resolve('some-custom-directory/directory'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand All @@ -151,7 +151,7 @@ describe('writeTestResult', () => {
await writeTestResult(
{
titlePath: ['file.spec.ts', 'A grouping', 'Test Story'],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand All @@ -177,7 +177,7 @@ describe('writeTestResult', () => {
'.someFunction',
'.someFunction() calls something',
],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand All @@ -199,7 +199,7 @@ describe('writeTestResult', () => {
await writeTestResult(
{
titlePath: ['some.file.spec.ts', 'Test Story'],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand All @@ -221,7 +221,7 @@ describe('writeTestResult', () => {
await writeTestResult(
{
titlePath: ['some.file.cy.ts', 'Test Story'],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand All @@ -243,7 +243,7 @@ describe('writeTestResult', () => {
await writeTestResult(
{
titlePath: ['file.ts', 'Test Story'],
outputDir: resolve('test-results/test-story-chromium'),
outputDir: resolve('test-results'),
pageUrl: 'http://localhost:3000/',
viewport: { height: 800, width: 800 },
},
Expand Down
6 changes: 2 additions & 4 deletions packages/shared/src/write-archive/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { join } from 'path';
import { join } from 'node:path';
import { outputFile, ensureDir, outputJSONFile } from '../utils/filePaths';
import { logger } from '../utils/logger';
import { ArchiveFile } from './archive-file';
Expand Down Expand Up @@ -38,9 +38,7 @@ export async function writeTestResult(
);
// in Storybook, `/` splits the title out into hierarchies (folders)
const title = titlePathWithoutFileExtensions.join('/');
// outputDir gives us the test-specific subfolder (https://playwright.dev/docs/api/class-testconfig#test-config-output-dir);
// we want to write one level above that
const finalOutputDir = join(outputDir, '..', 'chromatic-archives');
const finalOutputDir = join(outputDir, 'chromatic-archives');

Copy link
Member

Choose a reason for hiding this comment

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

@tevanoff it seems like the directory traversal wouldn't be as much of a risk since people are configuring their own outputDir, which looks on "their" OS/container for files. Is it worth making this change to be totally safe, or is outputDir a relative path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skitterm yeah I don't think this is a change we need to make as we're not accepting this as an input from an unknown client.

const archiveDir = join(finalOutputDir, 'archive');

Expand Down
Loading