Skip to content

Commit

Permalink
Revert MediaHelper to only throw errors.
Browse files Browse the repository at this point in the history 
Change the Jest environment so in case of hook failure, the error is output to STDERR.
  • Loading branch information
@worldomonation
worldomonation committed Aug 31, 2021
1 parent 2f2672f commit a529ebc
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 4 deletions.
6 changes: 2 additions & 4 deletions packages/calypso-e2e/src/media-helper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,13 @@ export async function createTestFile(
try {
await fs.access( sourcePath );
} catch {
console.error( `Source file ${ sourcePath } not found on disk.` );
throw new Error();
throw new Error( `Source file ${ sourcePath } not found on disk.` );
}

// Obtain the file extension.
const extension = path.extname( sourcePath );
if ( ! extension ) {
console.error( `Extension not found on source file ${ sourcePath }` );
throw new Error();
throw new Error( `Extension not found on source file ${ sourcePath }` );
}

// Generate a filename using current timestamp and a pseudo-randomly generated integer.
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/lib/jest/environment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ class JestEnvironmentE2E extends JestEnvironmentNode {
break;

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

This comment has been minimized.

Sign in to view

Copy link
@scinos

scinos Aug 31, 2021

Contributor

We don't need it to log it for test_fn_failure?
Also I think we need __CURRENT_TEST_FAILED__ here as well, otherwise a hook failure won't trigger a screenshot

This comment has been minimized.

Sign in to view

Copy link
@worldomonation

worldomonation Aug 31, 2021 •
edited
Loading

Author Contributor

We don't need it to log it for test_fn_failure?

I haven't run into issues with errors not being thrown outside of the hooks, see example below:

  ● [WPCOM] Blocks: Media (Upload): (desktop) @parallel › Log in

    test error

      36 | 		const loginFlow = new LoginFlow( page, 'gutenbergSimpleSiteUser' );
      37 | 		await loginFlow.logIn();
    > 38 | 		throw new Error( 'test error' );
         | 		      ^
      39 | 	} );
      40 |
      41 | 	it( 'Start new post', async function () {

      at Object.<anonymous> (specs/specs-playwright/wp-blocks__media-spec.ts:38:9)

Also I think we need CURRENT_TEST_FAILED here as well, otherwise a hook failure won't trigger a screenshot

I did not set the value on purpose for hook failures taking into account the way we use hooks within the e2e suite (we only use beforeAll at the moment).

Also, if the value is set, the screenshot and video save codepath is taken in our custom hook, but since the browser instance is not fully ready, rename fails and this line outputs to STDERR. It can be confusing and lead someone down the wrong path (it did for me briefly while investigating an unrelated issue).

By not setting the __CURRENT_TEST_FAILED__ value I am trying to avoid this segment of the codepath from being run.
this.testFailed = true;
break;
case 'test_fn_failure':
this.global.__CURRENT_TEST_FAILED__ = true;
this.testFailed = true;
Expand Down

0 comments on commit a529ebc

Please sign in to comment.