Boost: Refactor and add new e2e tests

[davidlonjon]

Fixes #21756

Changes proposed in this Pull Request:

This PR implements the core E2E tests described in #21756. It is important to note there is not exactly a 1-1 relationship between the tests in this PR and the tests scenarios highlighted in #21756 but the tests in this PR will cover scenarios by doing a combination of steps and checks.

It adds some new Jetpack Boost WP CLI commands to allow managing tests prerequisites more easily. These commands

• A command to manage Jetpack Boost connection to WPCOM. it can be used to activate/deactivate and check the connection status.
• A command to clear any jetpack Boost cache, posts and related options to be able to bootstrap a clean Boost environment.

It creates a new prerequisite allowing setting up some test posts useful to test the Lazy Images and Defer JS modules. These test post pages can be accessed during a test via a new projects/plugins/boost/tests/e2e/lib/pages/TestContentPage.js. The appropriate test post page is retrieved using its id number which is retrieved using the post title. This is to avoid messing out and changing the permalinks structure since the repo files and therefore the .htaccess file are shared between the local development environment and the test environment. This might not be ideal but it seems to work pretty well.

This PR also make some refactoring and additions to the E2E Common files package to support the Jetpack Boost E2E tests. I tried to make them generic so they could be re-used in other plugins. These changes are:

• Adding the new selectJetpackBoost method to tools/e2e-commons/pages/wp-admin/sidebar.js to allow accessing the Jetpack Boost settings page. It could be contained inside the jetpack Boost E2E files but I thought it makes sense to keep all sidebar actions in that file.
• Refactoring the methods to activate and deactivate the Jetpack plugin from the Plugins page (using actions such as clicking on activate/deactivate links) in tools/e2e-commons/pages/wp-admin/plugins.js to make them more generic so they can be used to activate and inactivate any plugins.
• Add prerequisite actions to the tools/e2e-commons/env/prerequisites.js files to allow activating and deactivating plugins using the WP-CLI commands.

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

N/A

Testing instructions:

• The Jetpack Boost E2E GitHub action should pass.

Locally it can be tested doing the following:

• run pnpm install at the root of the repo
• run jetpack install plugins/jetpack
• run jetpack install plugins/boost
• in projects/plugins/boost/tests/e2e run pnpm install
• then in that same folder run pnpm run test-decrypt-config
• run pnpm run env-start && pnpm run tunnel-on
• and finally run pnpm run test-e2e

Also individual specs file can be run using for the command pnpm run test-e2e -- path/to/the/spec/file for example pnpm run test-e2e -- specs/speed-score.test.j.

Also the debug mode can be activated using the following command PWDEBUG=1 pnpm run test-e2e -- path/to/the/spec/file or PWDEBUG=1 pnpm run test-e2e for the whole test suite. The PWDEBUG=1 statement makes sure Chromium becomes visible so you can see the actual tests being executed. You will have to manually move some tests step forward from the Playwright Inspector window. See the video below:

CleanShot.2021-12-09.at.14.40.15.mp4

Finally, you once you did the original pnpm run env-start && pnpm run tunnel-on command, you can access your test site from any browser using the tunnel URL provided by the pnpm run tunnel-on command.

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ⚠️ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Boost plugin:

• Next scheduled release: January 31, 2022.
• Scheduled code freeze: January 24, 2022.

[davidlonjon]

@thingalon, I pushed a couple of commits since your last review to fix a typo and also to improve (a little) some of the horrific names of the Jetpack Boost page methods.

[karthikax]

Great work @davidlonjon, Thanks for this PR!

I was reviewing and testing this PR. Everything works great (again thanks for the testing instructions) with PWDEBUG=true.

However, when I run in headless mode some tests fail. Eg: specs/modules-common.test.js:44:3 › Modules › The render-blocking-js module state should revert back to original state. Adding some dummy timeouts solved the issue. Maybe this is something specific to my setup. So, I will leave it for others to review this as well.

Overall, it is amazing to see a developed test suite for Boost!!

[adimoldovan]

@davidlonjon, this looks great!

I have two concerns though:

1. The use of test.describe.serial. This is normally used to group tests that depend on each other, ensuring they run in the correct order. Looking at the tests, they seem to be independent of each other, so a serial run shouldn't be necessary. One big disadvantage to serial mode is that one failure will cause the next tests to be skipped.
   I'm not sure about your intention with this (maybe to make sure they won't run in parallel?), but either way, I strongly suggest not creating a dependency between tests.

2. Tests stability. I ran the suite a few times, both locally and in CI by re-running the job and every 2-3 runs there seems to be a failure. Tests related to Critical CSS and connection seem to be the most common failing.
   This is very important because an unstable test will very quickly lose its value.
   This is of course something that can closely be monitored and addressed later on. We're using this dashboard to monitor test stability in time.

[thingalon]

I've fixed a couple of issues which affected stability:

• In edea4ec, I fixed the way we use waitForLoadState. We're supposed to call it after triggering navigation, not before. This would have created a race condition, as were not (really) waiting for navigation to finish before pressing on after some actions (plugin activate, deactivate, login).
• In 4bd8587, I fixed a missing prerequisite; you shouldn't try to connect Jetpack when the plugin isn't active :)
• In abad901, I removed test.describe.serial; it's not necessary and the tests run fine without it.

Since applying these fixes, I'm finding it a lot more stable. @adimoldovan, really hoping to get this merged soon so that other members of my team can implement some additional tests while David is still here to help us through it. :)

[thingalon]

This is great!

I've tidied up a few inconsistency issues, and now it seems to reliably succeed every time for me.

[adimoldovan]

@thingalon, @davidlonjon Let's merge it!

[haqadn]

test.describe.serial is actually needed on Modules test suit @thingalon.

I have also found that sometimes the lazy-images revert fails for me as the waitForApiResponse is not effective in its case. I am pushing a couple of changes to fix these issues.

[haqadn]

Actually, serial makes sense here. The default, toggle and revert depend on the sequence for each module.

[pyronaur]

Just cc'ing @adimoldovan to make sure that makes sense :)

[haqadn]

To add some context, serial was used in multiple tests, and @adimoldovan's objection is valid. I only found on one test suit Modules where using serial was appropriate.

[adimoldovan]

Yeah, it looks like it. It's pretty confusing and I strongly suggest moving away from it.

I suggest merging the 3 tests into a single one and using steps for each action:

test( 'Toggle modules', async () => {
    test.step( `The ${ moduleSlug } module should be ${ moduleState } by default`, async () => {}
    test.step( `The ${ moduleSlug } module state should toggle to an inverse state`, async () => {}
    test.step( `The ${ moduleSlug } module state should revert back to original state, async () => {}
}

test.describe can be removed completely.

[adimoldovan]

Or better:

test( 'Toggle ${ moduleSlug } module', async () => {
    test.step( `The module should be ${ moduleState } by default`, async () => {}
    test.step( `The module state should toggle to an inverse state`, async () => {}
    test.step( `The module state should revert back to original state, async () => {}
}

[haqadn]

👍🏼

[davidlonjon]

Thank you all for the contribution to this PR and particularly @adimoldovan for having supported me all the way on this E2E journey for Boost. I am truly thankful for the guidance and kindness 🙏