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

ci(e2e): Refactor Media helper to better handle concurrency #55678

Merged
merged 13 commits into from
Aug 25, 2021
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
1 change: 0 additions & 1 deletion .teamcity/_self/projects/WebApp.kt
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,6 @@ fun playwrightBuildType( viewportName: String, buildUuid: String ): BuildType {
xvfb-run yarn jest --reporters=jest-teamcity --reporters=default --testNamePattern @parallel --maxWorkers=%E2E_WORKERS% specs/specs-playwright
""".trimIndent()
dockerImage = "%docker_image_e2e%"
dockerRunParameters = "-u %env.UID% --security-opt seccomp=.teamcity/docker-seccomp.json --shm-size=8gb"
}
bashNodeScript {
name = "Collect results"
Expand Down
4 changes: 1 addition & 3 deletions packages/calypso-e2e/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,9 @@
},
"devDependencies": {
"@types/config": "^0.0.39",
"@types/fs-extra": "^9.0.12",
"@types/jest": "^25.2.3",
"@types/node": "^15.0.2",
"asana-phrase": "^0.0.8",
"fs-extra": "3.0.1"
"asana-phrase": "^0.0.8"
},
"scripts": {
"clean": "yarn build --clean && npx rimraf dist",
Expand Down
43 changes: 16 additions & 27 deletions packages/calypso-e2e/src/media-helper.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import fs from 'fs/promises';
import os from 'os';
import path from 'path';
import config from 'config';
import fs from 'fs-extra';
import { getTimestamp } from './data-helper';

const artifacts: { [ key: string ]: string } = config.get( 'artifacts' );
Expand Down Expand Up @@ -41,31 +42,21 @@ export function getVideoDir(): string {
return path.resolve( getAssetDir(), process.env.VIDEODIR || artifacts.video );
}

/**
* Given a full path to file on disk, remove the file.
*
* @param {string} filePath Full path on disk.
* @returns {void} No return value.
*/
export function deleteFile( filePath: string ): void {
fs.removeSync( filePath );
}

/**
* Creates a temporary test file by cloning a source file under a new name.
*
* @param {{[key: string]: string}} param0 Parameter object.
* @param {string} param0.sourceFileName Basename of the source file to be cloned.
* @param {string} [param0.testFileName] Basename of the test file to be generated.
* @returns {string} Full path to the generated test file.
* @returns {Promise<string>} Full path to the generated test file.
*/
export function createTestFile( {
export async function createTestFile( {
sourceFileName,
testFileName,
}: {
sourceFileName: string;
testFileName?: string;
} ): string {
} ): Promise< string > {
let fileName = getTimestamp();
// If the output `testFileName` is defined, use that as part of the final filename.
if ( testFileName ) {
Expand All @@ -78,39 +69,37 @@ export function createTestFile( {
const sourceFileDir = path.join( __dirname, '../../../../../test/e2e/image-uploads/' );
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 outside the scope of this PR, but we should really refactor this out. A package shouldn't need to have knowledge of the directory structure of the project using said package. Instead, this path should come as part of sourceFileName

Copy link
Contributor

Choose a reason for hiding this comment

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

With the EPERM issues resolved, another solution (that was originally intended) could be to have an image-source directory in this package which holds the test files.

How does that sound?

const sourceFilePath = path.join( sourceFileDir, sourceFileName );

// Generated test file will also go under the source directory.
// Attempting to copy the file elsewhere will trigger the following error on TeamCity:
// EPERM: operation not permitted
const testFilePath = path.join( sourceFileDir, fileName );
// Copy the source file specified to testFilePath, creating a clone differing only by name.
fs.copySync( sourceFilePath, testFilePath );
const tempDir = await fs.mkdtemp( path.join( os.tmpdir(), 'e2e-' ) );
const testFilePath = path.join( tempDir, fileName );

await fs.copyFile( sourceFilePath, testFilePath );

return testFilePath;
}

/**
* Returns the path to a generated temporary JPEG image file.
*
* @returns {string} Full path on disk to the generated test file.
* @returns {Promise<string>} Full path on disk to the generated test file.
*/
export function createTestImage(): string {
return createTestFile( { sourceFileName: 'image0.jpg' } );
export async function createTestImage(): Promise< string > {
return await createTestFile( { sourceFileName: 'image0.jpg' } );
}

/**
* Returns the path to a generated temporary MP3 audio file.
*
* @returns {string} Full path on disk to the generated test file.
*/
export function createTestAudio(): string {
return createTestFile( { sourceFileName: 'bees.mp3' } );
export async function createTestAudio(): Promise< string > {
return await createTestFile( { sourceFileName: 'bees.mp3' } );
}

/**
* Returns the path to an unsupported file.
*
* @returns {string} Full path on disk to the generated test file.
*/
export function createInvalidFile(): string {
return createTestFile( { sourceFileName: 'unsupported_extension.mkv' } );
export async function createInvalidFile(): Promise< string > {
return await createTestFile( { sourceFileName: 'unsupported_extension.mkv' } );
}
11 changes: 9 additions & 2 deletions test/e2e/specs/specs-playwright/wp-blocks__coblocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,22 @@ describe( DataHelper.createSuiteTitle( 'Blocks: CoBlocks' ), () => {
let gutenbergEditorPage: GutenbergEditorPage;
let pricingTableBlock: PricingTableBlock;
let page: Page;
let logoImage: string;

// Test data
const pricingTableBlockPrice = 888;
const heroBlockHeading = 'Hero heading';
const clicktoTweetBlockTweet =
'The foolish man seeks happiness in the distance. The wise grows it under his feet. — James Oppenheim';
const logoImage = MediaHelper.createTestImage();

setupHooks( ( args ) => {
page = args.page;
} );

beforeAll( async () => {
logoImage = await MediaHelper.createTestImage();
} );

it( 'Log in', async function () {
const loginFlow = new LoginFlow( page, 'gutenbergSimpleSiteUser' );
await loginFlow.logIn();
Expand Down Expand Up @@ -87,12 +91,15 @@ describe( DataHelper.createSuiteTitle( 'Blocks: CoBlocks' ), () => {
${ DynamicHRBlock } | ${ null }
${ HeroBlock } | ${ [ heroBlockHeading ] }
${ ClicktoTweetBlock } | ${ [ clicktoTweetBlockTweet ] }
${ LogosBlock } | ${ [ path.parse( logoImage ).name ] }
`(
`Confirm $block.blockName block is visible in published post`,
async ( { block, content } ) => {
// Pass the Block object class here then call the static method to validate.
await block.validatePublishedContent( page, content );
}
);

it( `Confirm Logos block is visible in published post`, async () => {
await LogosBlock.validatePublishedContent( page, path.parse( logoImage ).name );
} );
} );
55 changes: 30 additions & 25 deletions test/e2e/specs/specs-playwright/wp-blocks__media-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,23 @@ import { Page } from 'playwright';
describe( DataHelper.createSuiteTitle( 'Blocks: Media (Upload)' ), () => {
let gutenbergEditorPage: GutenbergEditorPage;
let page: Page;

const testFiles = {
image: MediaHelper.createTestImage(),
image_reserved_name: MediaHelper.createTestFile( {
sourceFileName: 'image0.jpg',
testFileName: 'filewith#?#?reservedurlchars',
} ),
audio: MediaHelper.createTestAudio(),
};
let testFiles: { image: string; image_reserved_name: string; audio: string };

setupHooks( ( args ) => {
page = args.page;
} );

beforeAll( async () => {
testFiles = {
image: await MediaHelper.createTestImage(),
image_reserved_name: await MediaHelper.createTestFile( {
sourceFileName: 'image0.jpg',
testFileName: 'filewith#?#?reservedurlchars',
} ),
audio: await MediaHelper.createTestAudio(),
};
} );

it( 'Log in', async function () {
const loginFlow = new LoginFlow( page, 'gutenbergSimpleSiteUser' );
await loginFlow.logIn();
Expand Down Expand Up @@ -72,20 +75,22 @@ describe( DataHelper.createSuiteTitle( 'Blocks: Media (Upload)' ), () => {
await gutenbergEditorPage.publish( { visit: true } );
} );

// Pass in a 1D array of values or text strings to validate each block.
// The full filename (name.extension) is not used within the Image block, but the file name is.
// `path.parse` is called to trim the extension.
it.each`
block | content
${ ImageBlock } | ${ [ path.parse( testFiles.image ).name ] }
${ ImageBlock } | ${ [ path.parse( testFiles.image_reserved_name ).name.replace( /[^a-zA-Z ]/g, '' ) ] }
${ AudioBlock } | ${ [ path.parse( testFiles.audio ).base ] }
${ FileBlock } | ${ [ path.parse( testFiles.audio ).name ] }
`(
`Confirm $block.blockName block is visible in published post`,
async ( { block, content } ) => {
// Pass the Block object class here then call the static method to validate.
await block.validatePublishedContent( page, content );
}
);
it( `Confirm Image block is visible in published post`, async () => {
await ImageBlock.validatePublishedContent( page, path.parse( testFiles.image ).name );
} );

it( `Confirm Image block is visible in published post (reserved name)`, async () => {
await ImageBlock.validatePublishedContent(
page,
path.parse( testFiles.image_reserved_name ).name.replace( /[^a-zA-Z ]/g, '' )
);
} );

it( `Confirm Audio block is visible in published post`, async () => {
await AudioBlock.validatePublishedContent( page );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AudioBlock.validatePublishedContent() doesn't check for the content, it only looks for the presence of the <audio> tag. This was hidden by the previous implementation as block was type any, but now it is correctly detected by TypeScript.

} );

it( `Confirm File block is visible in published post`, async () => {
await FileBlock.validatePublishedContent( page, path.parse( testFiles.audio ).name );
} );
} );
6 changes: 5 additions & 1 deletion test/e2e/specs/specs-playwright/wp-media__edit-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ import {
} from '@automattic/calypso-e2e';

describe( DataHelper.createSuiteTitle( 'Media: Edit Media' ), function () {
const testImage = MediaHelper.createTestImage();
let testImage;
let page;

setupHooks( ( args ) => {
page = args.page;
} );

beforeAll( async () => {
testImage = await MediaHelper.createTestImage();
} );

describe.each`
siteType | user
${ 'Simple' } | ${ 'defaultUser' }
Expand Down
40 changes: 19 additions & 21 deletions test/e2e/specs/specs-playwright/wp-media__upload-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,21 @@ import {
} from '@automattic/calypso-e2e';

describe( DataHelper.createSuiteTitle( 'Media: Upload' ), () => {
const testFiles = [
{ type: 'image', filepath: MediaHelper.createTestImage() },
{ type: 'audio', filepath: MediaHelper.createTestAudio() },
];
const invalidFile = MediaHelper.createInvalidFile();
let testFiles;
let page;

setupHooks( ( args ) => {
page = args.page;
} );

beforeAll( async () => {
testFiles = {
image: await MediaHelper.createTestImage(),
audio: await MediaHelper.createTestAudio(),
invalid: await MediaHelper.createInvalidFile(),
};
} );

// Parametrized test.
describe.each`
siteType | user
Expand All @@ -42,28 +46,22 @@ describe( DataHelper.createSuiteTitle( 'Media: Upload' ), () => {
mediaPage = new MediaPage( page );
} );

it.each( testFiles )(
'Upload $type and confirm addition to gallery',
async ( { filepath } ) => {
const uploadedItem = await mediaPage.upload( filepath );
assert.strictEqual( await uploadedItem.isVisible(), true );
}
);
it( 'Upload image and confirm addition to gallery', async () => {
const uploadedItem = await mediaPage.upload( testFiles.image );
assert.strictEqual( await uploadedItem.isVisible(), true );
} );

it( 'Upload audio and confirm addition to gallery', async () => {
const uploadedItem = await mediaPage.upload( testFiles.audio );
assert.strictEqual( await uploadedItem.isVisible(), true );
} );

it( 'Upload an unsupported file type and see the rejection notice', async function () {
try {
await mediaPage.upload( invalidFile );
await mediaPage.upload( testFiles.invalid );
} catch ( error ) {
assert.match( error.message, /could not be uploaded/i );
}
} );
} );

// Clean up test files.
afterAll( () => {
for ( const testFile of Object.values( testFiles ) ) {
MediaHelper.deleteFile( testFile.filepath );
}
MediaHelper.deleteFile( invalidFile );
} );
} );
23 changes: 0 additions & 23 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3926,13 +3926,6 @@
resolved "https://registry.yarnpkg.com/@types/fast-json-stable-stringify/-/fast-json-stable-stringify-2.0.0.tgz#40363bb847cb86b2c2e1599f1398d11e8329c921"
integrity sha512-mky/O83TXmGY39P1H9YbUpjV6l6voRYlufqfFCvel8l1phuy8HRjdWc1rrPuN53ITBJlbyMSV6z3niOySO5pgQ==

"@types/fs-extra@^9.0.12":
version "9.0.12"
resolved "https://registry.yarnpkg.com/@types/fs-extra/-/fs-extra-9.0.12.tgz#9b8f27973df8a7a3920e8461517ebf8a7d4fdfaf"
integrity sha512-I+bsBr67CurCGnSenZZ7v94gd3tc3+Aj2taxMT4yu4ABLuOgOjeFxX3dokG24ztSRg5tnT00sL8BszO7gSMoIw==
dependencies:
"@types/node" "*"

"@types/glob-base@^0.3.0":
version "0.3.0"
resolved "https://registry.yarnpkg.com/@types/glob-base/-/glob-base-0.3.0.tgz#a581d688347e10e50dd7c17d6f2880a10354319d"
Expand Down Expand Up @@ -13299,15 +13292,6 @@ fs-exists-sync@^0.1.0:
resolved "https://registry.yarnpkg.com/fs-exists-sync/-/fs-exists-sync-0.1.0.tgz#982d6893af918e72d08dec9e8673ff2b5a8d6add"
integrity sha1-mC1ok6+RjnLQjeyehnP/K1qNat0=

fs-extra@3.0.1:
version "3.0.1"
resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-3.0.1.tgz#3794f378c58b342ea7dbbb23095109c4b3b62291"
integrity sha1-N5TzeMWLNC6n27sjCVEJxLO2IpE=
dependencies:
graceful-fs "^4.1.2"
jsonfile "^3.0.0"
universalify "^0.1.0"

fs-extra@^0.22.1:
version "0.22.1"
resolved "https://registry.yarnpkg.com/fs-extra/-/fs-extra-0.22.1.tgz#5fd6f8049dc976ca19eb2355d658173cabcce056"
Expand Down Expand Up @@ -17351,13 +17335,6 @@ jsonfile@^2.1.0:
optionalDependencies:
graceful-fs "^4.1.6"

jsonfile@^3.0.0:
version "3.0.1"
resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-3.0.1.tgz#a5ecc6f65f53f662c4415c7675a0331d0992ec66"
integrity sha1-pezG9l9T9mLEQVx2daAzHQmS7GY=
optionalDependencies:
graceful-fs "^4.1.6"

jsonfile@^4.0.0:
version "4.0.0"
resolved "https://registry.yarnpkg.com/jsonfile/-/jsonfile-4.0.0.tgz#8771aae0799b64076b76640fca058f9c10e33ecb"
Expand Down