Site Editor: Avoid depending on specific themes for tests

[david-szabo97]

Note: This is mainly about Site Editor, but it might also apply to other editors as well. Did anyone bump into issues while working on non-site-editor parts of Gutenberg?

Context

A few days ago I noticed we are depending on TT1B theme for a few PHP unit tests:

gutenberg/phpunit/class-block-templates-test.php

Line 16 in d944866

switch_theme( 'tt1-blocks' );

gutenberg/phpunit/class-wp-rest-template-controller-test.php

Line 15 in e40a88c

switch_theme( 'tt1-blocks' );

gutenberg/packages/e2e-tests/specs/performance/site-editor.test.js

Line 25 in 37e1579

await activateTheme( 'tt1-blocks' );

And a few more...

By default, WordPress tests use the default theme, which is the latest core theme. (twenty-twenty, TT1, etc.) So when we aren't using switch_theme then we are running tests against TT1 theme.

The problem

We had a few changes in the site editor lately that we couldn't deliver without making changes in the TT1B theme. This means PRs can get blocked because we are depending on external dependencies (TT1B in this case). You can guess this lengthens the time it takes to deliver a PR since we need to wait for the changes in TT1B.

Running tests against the core theme has its own pro and con:
pro: since we are running the test against the core theme, we can ensure that Gutenberg works with the core theme (or TT1B in case of site editor)
con: if we want to make some changes, we need to coordinate it with the theme

Solution

A theme "fixture" that evolves with Gutenberg. Rather than using an external theme (TT1B), we should create a very minimal theme. We also gain a commit history where we can see what changes we had to make in this fixture theme to make it compatible with Gutenberg.

Implementation

We can create a blocks-theme directory in the repo and map it in .wp-env

	"themes": [ "./blocks-theme", "WordPress/theme-experiments/tt1-blocks" ],

Then we just need to implement the minimum to get the site editor working.

[ockham]

I'll add that even more disruptively, Gutenberg's end-to-end tests were broken for a while after TT1 Blocks removed their front-page template (WordPress/theme-experiments#179) -- which our e2e tests relied on. This was thankfully fixed on the Gutenberg side by @Addison-Stavlo in #28638.

However, it's evidence that we shouldn't rely on external dependencies for our day-to-day quality assurance -- in the form of e2e tests, which we want people to trust, rather than ignore.

[ockham]

@jeyip and I just discussed this in Slack. We came to the conclusion that the key issue here is reproducibility: Depending on a moving target -- such as the https://github.com/WordPress/theme-experiments repo -- makes unpredictable with what actual theme code we end up with (since that implicitly refers to the HEAD of the master branch of that repo, which is constantly changing).

gutenberg/.wp-env.json

Line 6 in 0105c9a

"themes": [ "WordPress/theme-experiments/tt1-blocks" ],

A viable fix for this might be to pin this to a certain version instead. Fortunately, it seems like wp-env supports using git refs (such as commit SHAs and branch or tag names).

So we might actually get away with pinning to e.g. WordPress/theme-experiments@f3d2c0d (and maybe we can later convince theme-experiments folks to tag theme versions, e.g. [email protected]).

One final caveat: I'm not 100% sure if wp-env supports both pinning a GH repo to a given ref, and specifying a subdirectory of that repo (as we need to do here for tt1-blocks).

[ockham]

Well wp-env's config parsing seems to work anyway 🤔 (Tests pass with the following change.)

diff --git a/packages/env/lib/config/test/config.js b/packages/env/lib/config/test/config.js
index c9d620b12d..a20bf54569 100644
--- a/packages/env/lib/config/test/config.js
+++ b/packages/env/lib/config/test/config.js
@@ -366,6 +366,7 @@ describe( 'readConfig', () => {
                                                        'WordPress/gutenberg',
                                                        'WordPress/gutenberg#master',
                                                        'WordPress/gutenberg#5.0',
+                                                       'WordPress/theme-experiments/tt1-blocks#f3d2c0d',
                                                ],
                                        } )
                                )
@@ -394,6 +395,16 @@ describe( 'readConfig', () => {
                                                path: expect.stringMatching( /^\/.*gutenberg$/ ),
                                                basename: 'gutenberg',
                                        },
+                                       {
+                                               type: 'git',
+                                               url:
+                                                       'https://github.com/WordPress/theme-experiments.git',
+                                               ref: 'f3d2c0d',
+                                               path: expect.stringMatching(
+                                                       /^\/.*theme-experiments\/tt1-blocks$/
+                                               ),
+                                               basename: 'tt1-blocks',
+                                       },
                                ],
                        };
                        expect( config.env.tests ).toMatchObject( matchObj );

[carolinan]

This is a problem in communication about the purpose and scope of TT1 Blocks when it was created, as it was never even stated to the developers (hi) that this theme was meant to be used this way.

You can guess this lengthens the time it takes to deliver a PR since we need to wait for the changes in TT1B.
We need everyone to clearly express what they need, not sit and wait.

[ockham]

Hey @carolinan! 👋 Thanks for stopping by!

It's true that different stakeholders have different needs and use cases, and we don't always state those clearly when we start using each other's projects.

That said, I'll give a try to the pinned dependency approach I was outlining in my other comment, as I think it might be a viable solution.

So we might actually get away with pinning to e.g. WordPress/theme-experiments@f3d2c0d (and maybe we can later convince theme-experiments folks to tag theme versions, e.g. [email protected]).

Asked here: WordPress/theme-experiments#195