Refactor Site Editor test utils to the `e2e-test-utils` package #38463

sunyatasattva · 2022-02-02T21:15:12Z

Description

Currently, the e2e-tests packaged includes some utility functions to test the Site Editor. This, as confirmed through a chat with @youknowriad, was due to the fact that the Site Editor had been in an experimental phase for a long time.

Now that the Site Editor has shipped to Core, it is time to graduate those test utils to the appropriate e2e-test-utils package and document them, so that they can be shared also in other packages and repositories.

This is what this PR does: it extracted most of the utility functions into the e2e-test-utils, documented them and added some extra checks.

There might be some other functions which might be useful to refactor inside the test cases themselves, but I thought I'd first open a PR for the bulk of them.

More details on what has changed below.

Testing Instructions

This PR only refactors E2E tests. So to test it, just run: npm run test-e2e.

Types of changes

Mostly refactor. Any change introduced is non breaking. In particular, the following changes have been introduced to the utils API:

The existing toggleMoreMenu function now accepts an optional parameter called context (which indicates whether the button is in the Site Editor or Post Editor context). The reason is that the selector is different depending on the context. Perhaps in the future it could be nice to either: (a) have the same aria label for both buttons (b) change the selectors so that they coincide. For now, however, I kept the selector as I didn't want to go over scope with this PR.
The above function also accepts another optional parameter waitFor. If set, this allows to wait for the menu to be open or closed, making sure that the menu is in the state it is expected to be. The parameter is optional and if not provided won't wait for anything (previous behavior). I have, however, refactored all the instances of this function I found to make sure the tests were explicit on their expectations.
The existing clickOnMoreMenuItem function also accepts a context parameter for the reasons described above in (1).

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

That didn't seem to be used anywhere. Perhaps a bit of a red flag. Also decided not to go with adding an optional parameter to the existing `getEditedPostContent` because the two functions differed significantly. Perhaps it is not very consistent, however.

Function renamed to `getEditedPageContent` to avoid conflicts with existing functions. Also found instances in which it was used and replaced them.

…tions

The function can now wait for opening or closing as required, making the clicking of menu items more stable.

github-actions · 2022-02-02T21:15:34Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @sunyatasattva! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

youknowriad · 2022-02-03T08:34:01Z

packages/e2e-test-utils/README.md

@@ -258,6 +271,10 @@ Disable auto-accepting any dialogs.

 Disables Pre-publish checks.

+### disableWelcomeGuide


For the post editor we have a welcome guide as well and instead of a separate function it's an option in createNewPost. Might be good to do something similar or if we keep the separate function have it work in both contexts?

The action isn't unique to the Site Editor, and complexity will grow as we introduce more editors.

Let's make it part of the goToSiteEditor function and disable it by default.

youknowriad · 2022-02-03T08:35:13Z

packages/e2e-test-utils/README.md

@@ -364,6 +381,14 @@ _Returns_

 -   `Promise`: Promise resolving with current post content markup.

+### getEditedPageContent


I think the name of this function is a bit ambiguous, it's not clear that it's the content of the site editor, a page can also be shown in the post editor. Maybe getSiteEditorCurrentContent or getSiteEditorContent or something like that?

youknowriad

this is great to see, extracting these was long overdue. I'm pinging some folks that I know worked on these previously in case they have a say @Addison-Stavlo @andrewserong @Mamaduka

Mamaduka · 2022-02-03T09:08:30Z

packages/e2e-test-utils/README.md

+
+-   `Promise<?ElementHandle>`: The menu item handle or `null`
+
+### goToSiteEditor


suggestions (non-blocking): Maybe we follow the general visit* pattern for similar actions.

Mamaduka · 2022-02-03T11:08:34Z

packages/e2e-test-utils/src/site-editor.js

+/**
+ * Skips the welcome guide popping up to first time users of the site editor
+ */
+export async function disableWelcomeGuide() {


We probably don't want to export this function anymore and make it internal.

Mm… do you think so? I thought perhaps it might still be useful for some people to have it separately. Maybe they need to disable the welcome guide after doing some other steps?

I'm actually not sure. I'm confident you know best all the possible workflows, so if you say it would be useless, I'll remove the export and the reference in the docs.

In that case, let's add site in the function name. Since it only disables Site Editor welcome guides.

P.S. I don't think failing tests is related to this PR. See #38486 for details.

Good call! Done that.

P.S. I don't think failing tests is related to this PR. See #38486 for details.

Yea, they were passing alright before. Maybe after pushing the new commit they'll run again and work 🤞

Mamaduka

I think this is in good shape to land. The feedback got addressed, and all tests are passing.

Great work, @sunyatasattva 🥇

github-actions · 2022-02-04T06:44:54Z

Congratulations on your first merged pull request, @sunyatasattva! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

sunyatasattva · 2022-02-04T16:33:22Z

Thanks for being super welcoming @Mamaduka! Have a great weekend!

kevin940726 · 2022-02-07T03:24:55Z

Thank you for making this! Skimming through the diff got me thinking of whether we should encourage doing this though. The original utils are written in a Page Object Model style, which in my opinion is more suitable in our case.

e2e-test-utils is too bloated now and not friendly and discoverable for contributors to work on. To avoid repeating ourselves, one has to read through the big chunk of utils before writing the tests. With Page Object Model, we only have to look at the model of the current page to discover useful utils.

I'd even suggest inlining most utils directly in tests, I don't think it's an anti-pattern to repeat ourselves in tests, as long as the code is straightforward to read. We can make use of labeled block statement and comments to group and document pieces of code.

I'm currently trying to migrate from puppeteer to playwright, and refactor some of the testing styles in the meantime. I'll share more insights into what I think could make our tests better to read and write.

sunyatasattva · 2022-02-07T03:33:28Z

Hey @kevin940726 thanks for the comment.

I am totally a fan of the Page Object Model, and do agree 100% that it is the most suitable model. I have even written a small library to work with POs in Puppeteer, which I will open source once I finish the test coverage. So, I'm totally on board with you there.

For the scope of this PR, however, I had discussed with @youknowriad and also a few other people involved, and we decided to just follow the current pattern for the utils, and that probably doing a refactor was not worth the cost, and definitely beyond the scope of this PR.

You can still see my inspiration to POs in things like grouping SELECTORS into their own object to improve readability. I do agree that the current style could be improved in terms of readability, discoverability and just API access.

I'd even suggest inlining most utils directly in tests, I don't think it's an anti-pattern to repeat ourselves in tests, as long as the code is straightforward to read.

Not that my opinion for this project counts much, but that's the only part where I disagree. I have worked with tests a lot in the past and creating sharable utilities was a huge boon to the entire team. As long as the utilities are readable, discoverable and well documented, I personally don't see a big reason to reinvent the wheel (and, in fact, I would abstract a lot of what I saw here).

I'd be totally willing to help on that refactoring project, by the way!

kevin940726 · 2022-02-07T04:06:07Z

@sunyatasattva

we decided to just follow the current pattern for the utils, and that probably doing a refactor was not worth the cost, and definitely beyond the scope of this PR.

Yeah, I totally agree we should follow the current style until we finalize a new one. I was just taking the opportunity to share my thoughts here 😆 .

As long as the utilities are readable, discoverable and well documented, I personally don't see a big reason to reinvent the wheel.

I agree, but it's often hard to make them readable and discoverable in my past experience. For simple things like "querying an element and click on it", I don't think we need utils for them. As long as we have an accessible and readable querying engine, which is what I'm working on right now 😉 . That's just my opinion though and I value yours equally as well!

I'd be totally willing to help on that refactoring project, by the way!

Awesome! I'll be sharing more progress in the #core-test Slack channel. Join us if you're interested! ❤️

sunyatasattva added 10 commits February 2, 2022 21:53

Refactor site editor utils from e2e-tests to e2e-test-utils

45b855d

Refactor toggleMoreMenu to allow for both site and post editors

dfc2b4d

Abstract GutenbergContext into own type

ddfc6d4

Refactor clickOnMoreMenuItem to accept editor context

d2bd236

Rename getEditedPostContent for site editor and replace instances

b266df0

Function renamed to `getEditedPageContent` to avoid conflicts with existing functions. Also found instances in which it was used and replaced them.

Refactor disableWelcomeGuide into own function

e251698

Refactor Site Editor navigation panel utility functions into own func…

ef88991

…tions

Add an extra option to toggleMoreMenu and fix tests

ad04660

The function can now wait for opening or closing as required, making the clicking of menu items more stable.

Fix more tools menu content selector

65be622

sunyatasattva requested review from ajitbohra, nerrad and ntwb as code owners February 2, 2022 21:15

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 2, 2022

Fix site editor more tools menu content selector

5e460c3

Mamaduka added [Tool] E2E Test Utils /packages/e2e-test-utils [Package] E2E Tests /packages/e2e-tests [Type] Enhancement A suggestion for improvement. labels Feb 3, 2022

youknowriad requested review from gziolo and kevin940726 February 3, 2022 08:27

youknowriad reviewed Feb 3, 2022

View reviewed changes

Mamaduka reviewed Feb 3, 2022

View reviewed changes

Rename some functions, disable welcome guide by default

a5ea6ae

Mamaduka reviewed Feb 3, 2022

View reviewed changes

Rename getEditedPageContent to getCurrentSiteEditorContent

0fa2f40

sunyatasattva requested review from youknowriad and Mamaduka February 3, 2022 12:05

Rename disableWelcomeGuide function

1f64527

Mamaduka approved these changes Feb 4, 2022

View reviewed changes

Mamaduka merged commit 60df02e into WordPress:trunk Feb 4, 2022

github-actions bot added this to the Gutenberg 12.6 milestone Feb 4, 2022

sunyatasattva deleted the refactor/site-editor-test-utils branch February 4, 2022 18:17

sunyatasattva mentioned this pull request Feb 4, 2022

E2E tests for Single Product Template woocommerce/woocommerce-blocks#5722

Merged

10 tasks

sunyatasattva mentioned this pull request Feb 8, 2022

E2E tests for Product Archive Template and Legacy Block woocommerce/woocommerce-blocks#5748

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Site Editor test utils to the `e2e-test-utils` package #38463

Refactor Site Editor test utils to the `e2e-test-utils` package #38463

sunyatasattva commented Feb 2, 2022 •

edited

Loading

github-actions bot commented Feb 2, 2022

youknowriad Feb 3, 2022

Mamaduka Feb 3, 2022

youknowriad Feb 3, 2022

youknowriad left a comment

Mamaduka Feb 3, 2022

Mamaduka Feb 3, 2022

sunyatasattva Feb 3, 2022

Mamaduka Feb 3, 2022

sunyatasattva Feb 3, 2022

Mamaduka left a comment •

edited

Loading

github-actions bot commented Feb 4, 2022

sunyatasattva commented Feb 4, 2022

kevin940726 commented Feb 7, 2022

sunyatasattva commented Feb 7, 2022

kevin940726 commented Feb 7, 2022

		@@ -258,6 +271,10 @@ Disable auto-accepting any dialogs.

		Disables Pre-publish checks.

		### disableWelcomeGuide

		@@ -364,6 +381,14 @@ _Returns_

		- `Promise`: Promise resolving with current post content markup.

		### getEditedPageContent


		- `Promise<?ElementHandle>`: The menu item handle or `null`

		### goToSiteEditor

Refactor Site Editor test utils to the e2e-test-utils package #38463

Refactor Site Editor test utils to the e2e-test-utils package #38463

Conversation

sunyatasattva commented Feb 2, 2022 • edited Loading

Description

Testing Instructions

Types of changes

Checklist:

github-actions bot commented Feb 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a 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

Mamaduka left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2022

sunyatasattva commented Feb 4, 2022

kevin940726 commented Feb 7, 2022

sunyatasattva commented Feb 7, 2022

kevin940726 commented Feb 7, 2022

Refactor Site Editor test utils to the `e2e-test-utils` package #38463

Refactor Site Editor test utils to the `e2e-test-utils` package #38463

sunyatasattva commented Feb 2, 2022 •

edited

Loading

Mamaduka left a comment •

edited

Loading