search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 286 forks source link

Implement testing infrastructure for Gutenberg integrations #4108

Open felixarntz opened 3 years ago

felixarntz commented 3 years ago

Based on #4107, this issue is focused on setting up the necessary infrastructure to test Gutenberg integrations as reliably as possible.

Some particular challenges to work around here include:

We need a testing infrastructure that caters for this. While it is impossible to cover all possible combinations of versions, we should have a good set to rely on. Potentially, we should introduce a subset of our e2e test suite specifically for Gutenberg-based e2e tests, where we would run for a few more scenarios (e.g. one pre-5.0 with Gutenberg plugin, one with 5.0, one with 5.3, one with latest). In addition to e2e tests (which will be the most reliable automatic way to test the integrations), a good additional measure could be to always use the exact versions used in WordPress 5.0 for all the Gutenberg dependencies that Site Kit is using even in Jest tests (at least this part will technically rely on #4107 to be completed first) - to ensure we don't use APIs/functions that were only supported at a later point.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

In this issues, "Gutenberg plugin" refers to the WP Gutenberg plugin that can be used to get the "latest" Gutenberg features/version, as referenced above.

Implementation Brief

To reduce the complexity of the test run code locally and improve local test speed, I think it's best to only run the various WordPress version tests (eg. 5.2 and latest) on GitHub Actions/CI. We should run the tests locally on one specific version, but this can be changed locally using existing tools. Testing mostly on latest is fine, and the CI tests will cover any APIs not available in 5.2. After exploring options during the Hackathon, I think this is the least complex and also much-faster-to-run approach.

In terms of testing with/without the Gutenberg plugin installed: I think this is best done by using our existing "enable plugin" approach, eg using activatePlugin (see an example of that helper function usage here: https://github.com/google/site-kit-wp/blob/6fb1e91b6b72f5aa029b28e47da00b13a70fba19/tests/e2e/specs/admin-tracking.test.js#L49). We should have a dedicated "Gutenberg test function" or utility that we can wrap a group of tests in that will run them both with and without the Gutenberg plugin. This will ensure the APIs used work in both the oldest, latest, and future versions of Gutenberg.

This means we won't need to create any dedicated E2E folders or configs for Gutenberg tests. This is contrary to the ACs, but I think is a simpler approach that means Gutenberg tests can be part of the existing tests. Otherwise, we essentially need extra environments, complex scripts, and multiple locations for E2E tests.

Test Coverage

QA Brief

Changelog entry

felixarntz commented 2 years ago

Thoughts on this one @aaemnnosttv @tofumatt @eugene-manuilov? Let's prioritize this for the new year to find a good approach.

tofumatt commented 2 years ago

I added some info to the issue/ACs based on our discussion awhile back. Un-assigning myself for now, but please assign me again if you want more input 🙂

felixarntz commented 2 years ago

@aaemnnosttv @tofumatt @eugene-manuilov Updated ACs for your review. If y'all agree, let's move it to IB.

aaemnnosttv commented 2 years ago

LGTM 👍

FlicHollis commented 1 year ago

Hi @tofumatt notice this has been picked up a little while ago. Are you still planning on working on this IB? If not might be good to un-assign yourself and let someone else pick up. Let me know what you think, Thanks!

tofumatt commented 1 year ago

@FlicHollis I had a look through what would be required here as a hackathon exploration, so I'll leave it assigned for now. 🙂

tofumatt commented 1 year ago

I've updated the ACs here to omit the section around WordPress versions before 5.2, since 5.2 is now our minimum supported WordPress version. (See: #5874)

FlicHollis commented 1 year ago

Hi @tofumatt when you are back online please could you add an estimate here. Thanks!

aaemnnosttv commented 1 year ago

@felixarntz does this make sense to prioritize right now seeing as we no longer have a planned integration in the block editor?

felixarntz commented 1 year ago

@aaemnnosttv Good question, it's still a good thing to work on, but not a P0 anymore. I'll update to a P1 ("Infrastructure" already has some notion of that this is not as important as e.g. a P1 bug or enhancement).

tofumatt commented 1 year ago

Marking this as a 19 estimate just because this involves E2E tests.

techanvil commented 1 year ago

Thanks @tofumatt, this IB is looking good. I have a few questions:

tofumatt commented 1 year ago

Indeed, I meant using activatePlugin to test with and without the latest Gutenberg plugin enabled. If possible we'd want to download the latest version of the plugin, or setup a system where we routinely include the latest version in our E2E plugins if that's not possible. I added a note about the helper usage though with an example 👍🏻

In terms of options to link to—nothing really as most of my exploration with anything more complex/complicated (eg. a dedicated set of Gutenberg E2E separate from the rest of the tests with a dedicated runner, etc.) all proven complicated/cumbersome to run and unsatisfactory. The summary I think I posted to the Hackathon slides was basically that our original ideas of creating a "dedicated" set of E2E tests for Gutenberg environments would be at best ergonomically awkward and more work to maintain, but I couldn't even get it working well compared to just adding tests that would run as part of the existing E2E suite which was straightforward. Given our minimum WP version always supports Gutenberg that's now easy to do so it's what I suggest. Sorry there's not much to show, but it'd mostly just be half-baked failing branches that never got off the ground 😅

Converting the existing integration is largely around being able to use more idiomatic APIs. I actually don't think that's a huge change given the APIs we'd use would be useSelect so straightforward to add… and the only other thing we'd need to do is a React.render() call for the div we use for that Idea Hub integration. It's not strictly required as part of this issue, fair enough, and I suppose we could add the test for that "component" without migrating it and then migrate it later—even benefitting from the E2E test being there if you like. I figured it was a relatively small part of this issue, but if you still think it's best to separate out let me know and I'll file a follow-up issue for that.

techanvil commented 1 year ago

Hey @tofumatt, thanks for clarifying, and making that addition to the IB.

With regard to the Idea Hub notice, I have had a more fundamental realisation - we are in fact removing Idea Hub, via https://github.com/google/site-kit-wp/issues/6235, so for one thing it's clearly not worth the effort of converting the integration when its about to be removed.

It does also bring into question the viability of this Gutenberg testing framework if we don't have any tests for it, unless there are some on the horizon. Do you know if there's anything planned that would make use of it? I am a bit wary about adding code that's not being used at all...

tofumatt commented 1 year ago

I was thinking the same thing shortly after updating this IB.

I think it's worth adding this infrastructure once we need it, and not before. So we agree 🙂

I think for now I'll remove the bit about Idea Hub and move this to the stalled column. I think the existing ACs/IB will serve as a good base for the future, but there's no sense in doing this now without a real test.

We could "mock up" an integration but I don't think that's a great idea.

Moving to "Stalled" for now 👍🏻

techanvil commented 1 year ago

Thanks @tofumatt, SGTM!

FlicHollis commented 1 year ago

Thank you @techanvil and @tofumatt I will remove from the sprint too.