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

Automate the testing of popular blocks in current and edge versions of GB #45048

Merged
merged 18 commits into from
Aug 28, 2020

Conversation

fullofcaffeine
Copy link
Contributor

@fullofcaffeine fullofcaffeine commented Aug 20, 2020

Changes proposed in this Pull Request

  • Automates steps 1 to 7 of the Good Mountain testing checklist but does not focus on testing individual blocks (this will come in subsequent PR(s));
  • Look for blocks on the page and also look for block errors on the page;
  • Takes screenshots from edge and non-edge editor and preview.
  • Do that for edge/non-edge sites across the most popular themes
  • Adds a new test account and several new test sites (the account credentials have been added to the encrypted config file)

Repeat that for the most popular themes in wpcom.

Testing instructions

  1. Setup your env to run Calypso E2E tests (search for "Automated end-to-end Testing") in the FG;
  2. Run with ./node_modules/.bin/mocha specs/wp-calypso-gutenberg-upgrade-spec.js*[0]
  3. Wait for the tests to finish. It might take a good while since it tests everything for each test site
  4. Make sure it didn't err, if it did, it might not be an actual failure, try running that specific step again. If it continues failing, let us know
  5. Look inside the screenshot*[1] folder for the screenshots. There should be a directory per test site name. You can find the list of test sites in the test file itself. They are all part of the same test account called gutenbergUpgradeUser. You can verify it (alongside the credentials) by decrypting the config file in this PR and looking for it in your config/local-decrypted.json file.

*[0] Make sure to cd into test/e2e first.
*[1] test/e2e/screenshots/

TODO

  • Figure out if setting up blocks in a wpcom site then copying the block markup to another renders the blocks correctly in the new site (it looks like it does! This means we can postpone for now the idea of switching gutenberg or applying stickers using a query param)
  • Populate tiled-gallery with sample images
  • Take screenshots of the editor and preview in the non-edge site
  • Take screenshots of the editor and preview in the edge site, after loading the blocks markup from non-edge
  • Research how to set it up as a scheduled test and to run it using the "Needs e2e testing Gutenberg edge" tag (we'll just let it run as part of the current e2e pipeline)

@matticbot
Copy link
Contributor

@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.

@@ -0,0 +1,14 @@
/**
* External dependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left those comments for now as I'm not sure if I'll change these files soon, but will remove in a cleanup before taking this PR out of the draft state.

return codeEditor.sendKeys( Key.CONTROL + 'v' );
}

async setBlocksCode( code ) {
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 not used anymore and will be removed.

);
}

async getBlocksCode() {
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 not used anymore and will be removed


// jetpack/layout-grid
await gEditorComponent.insertLayoutGrid();
const layoutGridDisplayedInEditor = await gEditorComponent.layoutGridDisplayedInEditor();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's quite a bit of repetition here and I don't like it. I'll see how to dry it up later.

@fullofcaffeine fullofcaffeine force-pushed the try/gutenberg-upgrade-e2e-tests branch 2 times, most recently from 6f0bdb0 to 5cc6140 Compare August 25, 2020 02:50
@fullofcaffeine fullofcaffeine marked this pull request as ready for review August 25, 2020 03:17
@fullofcaffeine fullofcaffeine linked an issue Aug 26, 2020 that may be closed by this pull request
4 tasks
@fullofcaffeine fullofcaffeine force-pushed the try/gutenberg-upgrade-e2e-tests branch 2 times, most recently from 856a68f to 790bfa1 Compare August 27, 2020 14:43
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Nice start here, this is probably ready for more final reviews? Shall we swap the in progress label?

Comment on lines 6 to 9
class BlogPostsBlockComponent extends GutenbergBlockComponent {}

BlogPostsBlockComponent.blockTitle = 'Blog Posts';
BlogPostsBlockComponent.blockName = 'a8c/blog-posts';
Copy link
Member

Choose a reason for hiding this comment

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

This seems more idiomatic:

Suggested change
class BlogPostsBlockComponent extends GutenbergBlockComponent {}
BlogPostsBlockComponent.blockTitle = 'Blog Posts';
BlogPostsBlockComponent.blockName = 'a8c/blog-posts';
class BlogPostsBlockComponent extends GutenbergBlockComponent {
static blockTitle = 'Blog Posts';
static blockName = 'a8c/blog-posts';
}

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Aug 27, 2020

Choose a reason for hiding this comment

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

Oh, that was the first thing I tried. Unfortunately, I got a syntax error. The tests don't go through a transpiler, so it fallbacks to what node supports, and AFAIK, it still doesn't support static class variables yet.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Aug 27, 2020

Choose a reason for hiding this comment

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

Using node 12.18.0 (the one specified in wp-calypso's .nvmrc):

 ./node_modules/.bin/mocha specs/wp-calypso-gutenberg-upgrade-spec.js
file:///home/fullofcaffeine/workspace/code/wp-calypso/test/e2e/lib/gutenberg/blocks/youtube-block-component.js:7
        static blockTitle = 'Foo';
                   ^

SyntaxError: Invalid or unexpected token
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1176:10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, but it does support static getters:

class YoutubeBlockComponent extends GutenbergBlockComponent {
	static get blockTitle() {
		return 'foo ';
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll use static getters for now.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's an issue with how this is configured. I believe mocha is using its own parser. In node 12.18.0 it works:

> class A {}
undefined
> class B { a='a';}
undefined
> A.a
undefined
> B.a
undefined
> new B().a
'a'
> A.a='a'
'a'
> A.a
'a'
> class C { static a = 'a' }
undefined
> C.a
'a'

Let's just leave it for this PR. It's not really relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tested it with vanilla node and saw it worked. I tried to figure out what as the culprit in mocha but couldn't find and left it aside. I'm using static getters for now.

I'd be interested to know what is the culprit here. I saw the mocha js binary and AFAIK it uses node as well. That's pretty confusing.

Copy link
Member

@sirreal sirreal Aug 28, 2020

Choose a reason for hiding this comment

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

Fix for this in #45268. I added static properties in 509fec9.

@@ -0,0 +1,18 @@
// TODO some blocks are exported as default and some not... maybe change all of them to export as default?
Copy link
Member

Choose a reason for hiding this comment

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

Consistency is likely the best approach. Maybe in another PR we change them all to use one or the other.

Slight preference from me for named exports here.

@@ -10,10 +10,6 @@ import * as driverHelper from '../../driver-helper';
import GutenbergBlockComponent from './gutenberg-block-component';

export class ImageBlockComponent extends GutenbergBlockComponent {
constructor( driver, blockID ) {
Copy link
Member

Choose a reason for hiding this comment

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

This cleanup is nice, but it distracts from the focus on this PR. There's a lot of cleanup like this that we can probably do in a few tiny, focused PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy that 👍🏻 , I'll keep that in mind.

class TiledGalleryBlockComponent extends GutenbergBlockComponent {
async uploadImages( filesDetails ) {
const fileInput = this.driver.findElement(
By.xpath( `//*[@id="${ this.blockID.slice( 1 ) }"]/div/div/div[3]/div[2]/input` )
Copy link
Member

Choose a reason for hiding this comment

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

😅 Oh boy… what's this targeting? Something in the media placeholder? Maybe a dedicated helper would be good?

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Aug 28, 2020

Choose a reason for hiding this comment

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

This selects the hidden file input inside the tile gallery. I've moved this to its own instance field with a more descriptive name. Let me know if you had anything else in mind.

@fullofcaffeine fullofcaffeine requested a review from a team August 28, 2020 00:56
@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 28, 2020
@fullofcaffeine
Copy link
Contributor Author

@Automattic/good-mountain @Automattic/e2e-test-reviewers Please have a look and let me know what you think.

Please do bear in mind that it might seem incomplete - the test data for most of the blocks are not being filled yet (only for two of them) - but that was a deliberate decision taken to allow us to finish the initial groundwork/exploration task earlier instead of having a big/long-running PR (this version already provides some value and we can use it to help us test the next GB version).

Also, the assertion for invalid blocks has been commented out for now due to an issue in the jetpack/tiled-gallery. I'm not sure how to proceed here and could use some input. I see a couple of alternatives: 1) We can ignore this step for the jetpack/tiled-gallery for now; 2) We can leave it commented out until the issue is fixed; 3) We can remove tiled-gallery from the checks?

@sirreal sirreal requested a review from a team August 28, 2020 08:24
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 28, 2020
@sirreal sirreal force-pushed the try/gutenberg-upgrade-e2e-tests branch from 509fec9 to 2f206ed Compare August 28, 2020 12:21
@sirreal
Copy link
Member

sirreal commented Aug 28, 2020

Rebased with #45268 merged.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Nice work here! 🚢

@sirreal
Copy link
Member

sirreal commented Aug 28, 2020

I don't see the screenshots artifacts in https://app.circleci.com/pipelines/github/Automattic/wp-e2e-tests-for-branches/36242/workflows/a894971d-692c-4e0e-9951-884404b1774c/jobs/97521/artifacts - is the plan to store them there?

We can introduce that in a follow-up, it should be a small change to the CircleCI configuration.

@fullofcaffeine
Copy link
Contributor Author

fullofcaffeine commented Aug 28, 2020

I don't see the screenshots artifacts in https://app.circleci.com/pipelines/github/Automattic/wp-e2e-tests-for-branches/36242/workflows/a894971d-692c-4e0e-9951-884404b1774c/jobs/97521/artifacts - is the plan to store them there?

We can introduce that in a follow-up, it should be a small change to the CircleCI configuration.

Hmm, thanks for pointing that out! I just assumed that by saving them to the screenshots folder, everything else would fall into place, but there might be something in CircleCI as you said. I'll take note of that.

@fullofcaffeine fullofcaffeine merged commit a8bab01 into master Aug 28, 2020
@fullofcaffeine fullofcaffeine deleted the try/gutenberg-upgrade-e2e-tests branch August 28, 2020 15:29
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 28, 2020
sirreal added a commit that referenced this pull request Aug 28, 2020
sirreal added a commit that referenced this pull request Aug 28, 2020
fullofcaffeine pushed a commit that referenced this pull request Aug 28, 2020
fullofcaffeine added a commit that referenced this pull request Aug 28, 2020
…ons of GB // take 2 (#45278)

* Revert "Revert "Automate the testing of popular blocks in current and edge versions of GB (#45048)" (#45277)"

This reverts commit 6c5580f.

* Update and encrypt fixed settings

Make sure it only adds the new Gutenberg upgrade account credentials and nothing else.

Co-authored-by: Marcelo Serpa <marcelo.serpa@automattic.com>
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.

[Gutenberg WPCOM e2e tests i0] Automate steps 1 to 7 (Tracking Issue)
3 participants