Playwright: use an interface for passing TestFile paths. #55693

worldomonation · 2021-08-24T21:10:49Z

Changes proposed in this Pull Request

This PR defines and implements a TypeScript interface to pass test file paths around.

Key changes:

interface defined to hold multiple parts of the filepath of a test file.
update test specs.

Testing instructions

Fixes #55641

matticbot · 2021-08-24T21:11:39Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot · 2021-08-24T21:19:25Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-14504
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-14504&env=jetpack

packages/calypso-e2e/src/media-helper.ts

worldomonation · 2021-08-24T21:27:56Z

packages/calypso-e2e/src/media-helper.ts

+	const extension = sourceFileName.split( '.' ).pop();
+	if ( ! extension ) {
+		throw new Error( `Extension not found on source file ${ sourceFileName }` );
+	}
+	const basename = `${ filename }.${ sourceFileName.split( '.' ).pop() }`;
+	// Create test files in the same directory as the source file.
+	const dirname = path.join( __dirname, '../../../../../test/e2e/image-uploads/' );
+	// Full path on disk of the source file, to be copied and renamed.
+	const sourceFilePath = path.join( dirname, sourceFileName );


Previous code did not survive the 3-month later check; I was confused by the reassignment of variables stemming from my attempts to limit the number of constants.

I've gone the other way now with this PR and creating single-use constants that better reflect the individual parts of the path, which can be assembled into an object implementing the interface at end.

worldomonation · 2021-08-25T23:29:18Z

test/e2e/specs/constants.js

+	'..',
+	'image-uploads',
+	'unsupported_extension.mkv'
+);


This file defines the paths where the test files are located.

Test writers should import the appropriate const and pass it into the createTestFile method.

worldomonation · 2021-08-25T23:30:05Z

test/e2e/specs/specs-playwright/wp-blocks__coblocks.ts

 		page = args.page;
 	} );

 	beforeAll( async () => {
-		logoImage = await MediaHelper.createTestImage();
+		logoImage = await MediaHelper.createTestFile( TEST_IMAGE_PATH );


All other methods for generating test files have been removed. Test writers are expected to pass the path of the source image file to the createTestFile method.

worldomonation · 2021-08-25T23:30:36Z

test/e2e/specs/specs-playwright/wp-media__edit-spec.ts

 	} );

 	describe.each`
 		siteType      | user
 		${ 'Simple' } | ${ 'defaultUser' }
 		${ 'Atomic' } | ${ 'wooCommerceUser' }
 	`( 'Edit Image ($siteType)', function ( { user } ) {
-		let mediaPage;
+		let mediaPage: MediaPage;


This file has been changed to TypeScript.

worldomonation · 2021-08-25T23:31:26Z

test/e2e/tsconfig.json

-	"include": [ "specs/specs-playwright" ] // TypeScript is scoped only for the new Playwright scripts
+		"types": [ "jest" ] // no mocha - we are only using TypeScript for the new Playwright scripts
+	},
+	"include": [ "specs/specs-playwright", "specs/constants.js" ] // TypeScript is scoped only for the new Playwright scripts


If the constants.js file is located within specs/specs-playwright, it causes Jest to attempt to execute the file as a test. This then fails due to the constants.js file not being a test.

scinos · 2021-08-26T03:15:30Z

packages/calypso-e2e/src/media-helper.ts

+	} = {}
+): Promise< TestFile > {
+	try {
+		await fs.open( sourcePath, 'r' );


Could you explain what are we trying to do here? Seems weird to open a file descriptor and not use it (plus we should close the fd later).

Is the goal to check that the file exists and is readable? In that case maybe we can use fs.access()

The intent is to check whether the file exists first.
With that said, it seems something else is swallowing the error - an invalid path given to this method didn't output stack trace as I would have expected.

It looks like throw Error is being swallowed since the code calling createTestFile is within a hook.

I've changed the throw statements to instead console.error and have verified this isn't swallowed up:

console.error Source file /Users/edwintakahashi/workspace/wp-calypso-third/test/e2e/specs/image-uploads/image0.jpg not found on disk. 80 | await fs.access( sourcePath ); 81 | } catch { > 82 | console.error( `Source file ${sourcePath} not found on disk.` ); | ^ 83 | // throw new Error( `Source file ${ sourcePath } not found on disk.` ); 84 | } 85 | at Object.createTestFile (../../packages/calypso-e2e/src/media-helper.ts:82:11) at specs/specs-playwright/wp-blocks__media-spec.ts:27:11

This is a common topic of discussion as shown here.

Fixed in 5d4b3c3.

scinos · 2021-08-26T03:18:16Z

packages/calypso-e2e/src/media-helper.ts

-	const tempDir = await fs.mkdtemp( path.join( os.tmpdir(), 'e2e-' ) );
-	const testFilePath = path.join( tempDir, fileName );
+	// Generate a filename using current timestamp and a pseudo-randomly generated integer.
+	let filename = `${ getTimestamp() }-${ getRandomInteger( 100, 999 ) }`;


fs.mkdtemp() "Creates a unique temporary directory. A unique directory name is generated by appending six random characters to the end of the provided prefix" (from the docs).

Do we still need to append a random integer to the filename?

The uniqueness here isn't about the directory/path per se, but rather has to do with the filename portion.

If two tests both end up creating a test file named identical_filename.jpg, uploading this to WPCOM would result in one instance of the file being renamed with a -1 postfix.
Later down the test, there are checks for whether the expected file name is found on the page. The -1 postfix interferes with this.

Another solution is to switch the validation to use a regex to match the important portion of the file name eg. 167888888.*.jpg.

Removed the postfix in cbd61f3.

scinos · 2021-08-26T03:20:01Z

packages/calypso-e2e/src/media-helper.ts

-export async function createTestAudio(): Promise< string > {
-	return await createTestFile( { sourceFileName: 'bees.mp3' } );
-}
+	await fs.copyFile( sourcePath, targetPath );


Maybe use mode COPYFILE_EXCL to fail if the target already exists.

Added in 5d4b3c3.

scinos · 2021-08-26T03:25:52Z

test/e2e/specs/constants.js

@@ -0,0 +1,10 @@
+import path from 'path';
+
+export const TEST_IMAGE_PATH = path.resolve( __dirname, '..', 'image-uploads', 'image0.jpg' );


The goal of path.resolve() is to find an absolute path appending from right to left, I think is misused here.

Instead we can use any of these:

path.join(__dirname, '../image-uploads/image0.jpg') path.normalize(`${__dirname}/../image-uploads/image0.jpg`)

I see, I had assumed the resolve would mean it joins all of them then resolves the resulting string to a path on the disk. Not the best name in my opinion.

I will choose path.normalize(`${__dirname}/../image-uploads/image0.jpg`).

Changed to a combination of path.normalize(path.join()) in 5d4b3c3.

worldomonation · 2021-08-30T22:46:06Z

That issue is 2 years old, and a few comments suggests errors work as expected with jest-circus (which we are already using).

What behaviour do you see with the throw?

This is what happens if throw is used:

  pw:api => browserType.launch started +0ms
  pw:api <= browserType.launch succeeded +1s
  pw:api => browser.newContext started +1ms
  pw:api <= browser.newContext succeeded +10ms
  pw:api => browserContext.newPage started +1ms
  pw:api <= browserContext.newPage succeeded +467ms
  pw:api => page.screenshot started +39ms
  pw:api <= page.screenshot succeeded +112ms
  pw:api => page.close started +1ms
  pw:api <= page.close succeeded +10ms
  pw:api => browser.close started +1ms
  pw:api <= browser.close succeeded +253ms
Test Suites: 1 skipped, 0 of 1 total
Tests:       12 skipped, 12 total
Snapshots:   0 total
Time:        2.833 s, estimated 28 s

Notice that Jest appears to be swallowing up the exception.

I can replicate this with this:

beforeAll( async () => {
		throw new Error( 'hook error' );
	} );

With console.error:

  console.error
    Source file /Users/edwintakahashi/workspace/wp-calypso/test/e2e/image-uploads/image0.jpg not found on disk.

      78 | 		await fs.access( '' );
      79 | 	} catch {
    > 80 | 		console.error( `Source file ${ sourcePath } not found on disk.` );
         | 		        ^
      81 | 	}
      82 |
      83 | 	// Obtain the file extension.

      at Object.createTestFile (../../packages/calypso-e2e/src/media-helper.ts:80:11)
      at specs/specs-playwright/wp-media__upload-spec.ts:24

worldomonation · 2021-08-30T22:47:09Z

Now, the way I had it wasn't correct either since console.error does not actually halt execution. I've fixed that with:

console.error('message')
throw new Error()

This way, the error message is not suppressed and the execution is halted as expected.

scinos · 2021-08-31T03:48:38Z

Looks like the problem is our custom environment.

As you found out, when a beforeHook throws an error it halts the execution but the actual message is never displayed. However, commenting out testEnvironment in test/e2e/jest.config.js does solve the problem (the error is shown):

Maybe we should have a console.error() in test/e2e/lib/jest/environment.js, in the failure cases

worldomonation · 2021-08-31T17:34:11Z

Looks like the problem is our custom environment.

As you found out, when a beforeHook throws an error it halts the execution but the actual message is never displayed. However, commenting out testEnvironment in test/e2e/jest.config.js does solve the problem (the error is shown):

Maybe we should have a console.error() in test/e2e/lib/jest/environment.js, in the failure cases

Thanks for the pointer. I only had a brief look at environment.js in the past and this was a good chance to look a bit deeper. This has the potential to do a lot of interesting things.

With my changes in a529ebc I see the following when a hook fails:

  pw:api => browserType.launch started +0ms
  pw:api <= browserType.launch succeeded +2s
  pw:api => browser.newContext started +1ms
  pw:api <= browser.newContext succeeded +7ms
  pw:api => browserContext.newPage started +2ms
  pw:api <= browserContext.newPage succeeded +488ms
Error: Source file /Users/edwintakahashi/workspace/wp-calypso/test/e2e/image-uploads/image0.jpg not found on disk.
    at Object.createTestFile (/Users/edwintakahashi/workspace/wp-calypso/packages/calypso-e2e/src/media-helper.ts:80:9)
    at /Users/edwintakahashi/workspace/wp-calypso/test/e2e/specs/specs-playwright/wp-media__upload-spec.ts:24:11
  pw:api => page.close started +31ms
  pw:api <= page.close succeeded +8ms
  pw:api => video.delete started +1ms
  pw:api <= video.delete succeeded +63ms
  pw:api => browser.close started +0ms
  pw:api <= browser.close succeeded +214ms
Test Suites: 1 skipped, 0 of 1 total
Tests:       12 skipped, 12 total
Snapshots:   0 total
Time:        3.227 s, estimated 4 s

What do you think?

scinos · 2021-09-02T18:27:45Z

I swear I added a comment on the changes in environment.js but I can't find it now :(
Sorry for the delay.

Once the comment about this.global.__CURRENT_TEST_FAILED__ has been addressed, this is ready to go.

scinos · 2021-09-02T18:30:47Z

test/e2e/lib/jest/environment.js

@@ -19,6 +19,9 @@ class JestEnvironmentE2E extends JestEnvironmentNode {
 				break;

 			case 'hook_failure':
+				console.error( event.error );


We don't want to log the error in case of test_fn_failure? Does jest does it automatically?

At the very least I think we should set this.global.__CURRENT_TEST_FAILED__ = true; here. That global property is checked by the screenshot/video recorder hooks.

Most of the time a failed hook mean there is nothing to take a screenshot from, but we can't guarantee that will be the case forever (for example, we may have a hook that opens the browser and does some navigation, and we do want to screenshot that if it fails).

I think my comment also went missing too 👀

We don't want to log the error in case of test_fn_failure? Does jest does it automatically?

In our current configuration, Jest automatically throws and outputs the relevant logs if the error is thrown outside of a hook.

At the very least I think we should set this.global.CURRENT_TEST_FAILED = true; here. That global property is checked by the screenshot/video recorder hooks.

Avoiding the screenshot/video recording codepath was why initially the value was not set. But as you mention:

we can't guarantee that will be the case forever (for example, we may have a hook that opens the browser and does some navigation, and we do want to screenshot that if it fails)

Yes, that is true. I will treat hook failure like a normal test failure in next commit.

Fixed in 0581a7d.

Revised behavior now outputs the following if anything in the hook fails:

FAIL specs/specs-playwright/wp-media__edit-spec.ts ● Test suite failed to run Source file /Users/edwintakahashi/workspace/wp-calypso-second/test/e2e/image-uploads/image0.jpg not found on disk. 78 | await fs.access( 'sourcePath' ); 79 | } catch { > 80 | throw new Error( `Source file ${ sourcePath } not found on disk.` ); | ^ 81 | } 82 | 83 | // Obtain the file extension. at Object.createTestFile (../../packages/calypso-e2e/src/media-helper.ts:80:9) at specs/specs-playwright/wp-media__edit-spec.ts:22:15 Test Suites: 1 failed, 1 total Tests: 0 total Snapshots: 0 total Time: 2.705 s

But I suspect throw event.error will prevent rest of the beforeAll or afterAll hook from executing, so this will likely not capture screenshots.

I've revised the behavior again in 87006be since I felt the previous behavior was a bit hacky.

Recap: throwing an error in environment.js causes rest of execution to halt. Practically, this means the afterAll hooks of screenshot/video capture are not run.

I've changed the behavior to instead do the following:

if a hook failure is detected, set a new attribute hookFailed.

at test_start event, check if hook has failed.

if hook has failed, mark all test cases in the describe block as failed.

I arrived at this solution because these lines were causing hook failures to be treated like a normal skip and ended up suppressing the errors thrown. To verify this, add an exception in a hook, modify the skip to a fail. Errors thrown in the hook should now be visible.

The separate treatment of hook failure is required, otherwise if an exception is thrown in the test step it would not be skipped. This can be verified by performing the same steps as above, but place the exception in the test step instead. Jest will not skip rest of the test steps.

Try pulling my changes in this revision, place exceptions in beforeAll hook, test step and afterAll hook. It should behave following this rule:

if exception is thrown in a hook, suite is marked as failed and test steps yet to run are also marked as failed.

if exception is thrown in a step, only the failing step is marked failed and rest are skipped.

Scenario 1:

● [WPCOM] Media: Edit Media: (desktop) @parallel › Edit Image (Atomic) › Cancel image edit hello! 20 | 21 | beforeAll( async () => { > 22 | throw new Error( 'hello!' ); | ^ 23 | testImage = await MediaHelper.createTestFile( TEST_IMAGE_PATH ); 24 | } ); 25 | at specs/specs-playwright/wp-media__edit-spec.ts:22:9 Test Suites: 1 failed, 1 total Tests: 16 failed, 16 total Snapshots: 0 total Time: 3.14 s, estimated 12 s

Scenario 2:

● [WPCOM] Media: Edit Media: (desktop) @parallel › Edit Image (Simple) › Log In hello! 31 | 32 | it( 'Log In', async function () { > 33 | throw new Error( 'hello!' ); | ^ 34 | const loginFlow = new LoginFlow( page, user ); 35 | await loginFlow.logIn(); 36 | } ); at Object.<anonymous> (specs/specs-playwright/wp-media__edit-spec.ts:33:10) Test Suites: 1 failed, 1 total Tests: 1 failed, 15 skipped, 16 total Snapshots: 0 total Time: 3.078 s, estimated 4 s

Scenario 3:

● Test suite failed to run hello! 60 | 61 | afterAll( async function () { > 62 | throw new Error( 'hello!' ); | ^ 63 | } ); 64 | } ); 65 | } ); at specs/specs-playwright/wp-media__edit-spec.ts:62:10 at runMicrotasks (<anonymous>) Test Suites: 1 failed, 1 total Tests: 2 passed, 2 total Snapshots: 0 total Time: 11.912 s, estimated 43 s

@scinos Could I please get a review on this before it gets too stale? Thanks!

scinos

🚢

- update Media (Upload) spec. - update CoBlocks spec. - update Blocks Media (Upload) spec.

Move Media Upload spec to TypeScript. Update Media (Edit) spec

- new file `constants.js` to hold the source paths for the test files. - call `createTestFile` by default and provide path. - refactor the createTestFile method to use better variable names. Revise behavior when hook fails. Revise environment behavior again. - set the `hookFailed` attribute to true if hook failure is experienced. - add clause at `test_start` event to check for hook failure, and if detected, mark rest of the test cases as failed. Rebase.

worldomonation force-pushed the pw/test-file-interface branch from 9879e6c to 72b7813 Compare August 24, 2021 21:12

worldomonation commented Aug 24, 2021

View reviewed changes

packages/calypso-e2e/src/media-helper.ts Outdated Show resolved Hide resolved

worldomonation commented Aug 24, 2021

View reviewed changes

worldomonation force-pushed the pw/test-file-interface branch from 5b6a182 to c348d1d Compare August 25, 2021 20:30

worldomonation commented Aug 25, 2021

View reviewed changes

This was referenced Aug 25, 2021

Playwright: add randomly generated integers as suffix for generated test files #55720

Closed

Playwright: Increase uniqueness of test filename #55717

Closed

worldomonation linked an issue Aug 25, 2021 that may be closed by this pull request

Playwright: Increase uniqueness of test filename #55717

Closed

worldomonation marked this pull request as ready for review August 25, 2021 23:39

worldomonation requested review from scinos, WunderBart and a team as code owners August 25, 2021 23:39

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 25, 2021

scinos reviewed Aug 26, 2021

View reviewed changes

worldomonation force-pushed the pw/test-file-interface branch from 3694103 to 5d4b3c3 Compare August 26, 2021 20:51

worldomonation self-assigned this Aug 30, 2021

worldomonation requested a review from scinos August 30, 2021 19:58

worldomonation force-pushed the pw/test-file-interface branch from a529ebc to 594a95a Compare September 2, 2021 16:20

scinos reviewed Sep 2, 2021

View reviewed changes

worldomonation requested a review from scinos September 6, 2021 17:20

scinos approved these changes Sep 8, 2021

View reviewed changes

worldomonation added 3 commits September 9, 2021 09:02

Define and implement interface for TestFile.

f270068

- update Media (Upload) spec. - update CoBlocks spec. - update Blocks Media (Upload) spec.

Correct comments referencing old way of obtaining file names.

1918804

Move Media Upload spec to TypeScript. Update Media (Edit) spec

worldomonation force-pushed the pw/test-file-interface branch from 87006be to eedb093 Compare September 9, 2021 16:08

worldomonation merged commit 091f787 into trunk Sep 9, 2021

worldomonation deleted the pw/test-file-interface branch September 9, 2021 16:58

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Playwright: use an interface for passing TestFile paths. #55693

Playwright: use an interface for passing TestFile paths. #55693

worldomonation commented Aug 24, 2021

matticbot commented Aug 24, 2021

matticbot commented Aug 24, 2021 •

edited

Loading

worldomonation Aug 24, 2021

worldomonation Aug 25, 2021

worldomonation Aug 25, 2021

worldomonation Aug 25, 2021

worldomonation Aug 25, 2021

scinos Aug 26, 2021

worldomonation Aug 26, 2021

worldomonation Aug 26, 2021

worldomonation Aug 26, 2021

scinos Aug 26, 2021

worldomonation Aug 26, 2021

worldomonation Aug 30, 2021

scinos Aug 26, 2021

worldomonation Aug 26, 2021

scinos Aug 26, 2021

worldomonation Aug 26, 2021 •

edited

Loading

worldomonation Aug 26, 2021

worldomonation commented Aug 30, 2021

worldomonation commented Aug 30, 2021 •

edited

Loading

scinos commented Aug 31, 2021 •

edited

Loading

worldomonation commented Aug 31, 2021

scinos commented Sep 2, 2021 •

edited

Loading

scinos Sep 2, 2021

worldomonation Sep 2, 2021

worldomonation Sep 2, 2021 •

edited

Loading

worldomonation Sep 2, 2021

worldomonation Sep 2, 2021

worldomonation Sep 7, 2021

scinos left a comment

		@@ -0,0 +1,10 @@
		import path from 'path';

		export const TEST_IMAGE_PATH = path.resolve( __dirname, '..', 'image-uploads', 'image0.jpg' );

Playwright: use an interface for passing TestFile paths. #55693

Playwright: use an interface for passing TestFile paths. #55693

Conversation

worldomonation commented Aug 24, 2021

Changes proposed in this Pull Request

Testing instructions

matticbot commented Aug 24, 2021

matticbot commented Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldomonation Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldomonation commented Aug 30, 2021

worldomonation commented Aug 30, 2021 • edited Loading

scinos commented Aug 31, 2021 • edited Loading

worldomonation commented Aug 31, 2021

scinos commented Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldomonation Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scinos left a comment

Choose a reason for hiding this comment

matticbot commented Aug 24, 2021 •

edited

Loading

worldomonation Aug 26, 2021 •

edited

Loading

worldomonation commented Aug 30, 2021 •

edited

Loading

scinos commented Aug 31, 2021 •

edited

Loading

scinos commented Sep 2, 2021 •

edited

Loading

worldomonation Sep 2, 2021 •

edited

Loading