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

Conversation

scinos
Copy link
Contributor

@scinos scinos commented Aug 24, 2021

Changes proposed in this Pull Request

Refactor Media Helper for Playwright tests to handle concurrency better:

  • createTestFile will use the temp directory
  • createTestFile is now async
  • Refactor usages of those methods to ensure they are called asynchronously inside a hook or a test
  • Drop code to delete files. They are saved in a tmp directory and disposed as soon as the docker container is killed.

After those changes, copying files was still breaking. Looks like removing --security-opt seccomp=.teamcity/docker-seccomp.json fixed the problem. I believe we added it for Electron 11 (https://github.com/Automattic/wp-calypso/pull/47555/files) but it is not required here.

Testing instructions

Verify e2e still pass

@github-actions
Copy link

github-actions bot commented Aug 24, 2021

@matticbot
Copy link
Contributor

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.

@scinos scinos force-pushed the chore/bump-fs-extra branch from b14c839 to 9ea22cf Compare August 25, 2021 05:05
@scinos scinos changed the title Bump fs-extra ci(e2e): Refactor Media helper to better handle concurrency Aug 25, 2021
@scinos scinos requested a review from worldomonation August 25, 2021 06:53
@matticbot 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 scinos requested review from WunderBart and a team and removed request for worldomonation August 25, 2021 06:53
@scinos scinos self-assigned this Aug 25, 2021
`(
`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 Logso block is visible in published post`, async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A side effect of the main change (move createTestImage inside a hook) is that we can't use logoImage to define a parameter anymore, as it doesn't exist when the test is defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bummer, but small price to pay for this change.

@@ -78,39 +79,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?

@scinos scinos marked this pull request as ready for review August 25, 2021 07:02
@scinos scinos requested a review from worldomonation as a code owner August 25, 2021 07:02
} );

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.

@scinos scinos force-pushed the chore/bump-fs-extra branch from cd08e2f to e19490b Compare August 25, 2021 10:21
@scinos scinos requested a review from a team August 25, 2021 10:21
@scinos scinos merged commit 082c2f6 into trunk Aug 25, 2021
@scinos scinos deleted the chore/bump-fs-extra branch August 25, 2021 17:29
@github-actions 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 Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants