WP-env test site uses a wrong theme's color palette making tests unreliable

afercia commented 1 year ago

Description

Noticed while working on https://github.com/WordPress/gutenberg/pull/42187

When using wp-env:

a development site is available at http://localhost:8888/
tests run on the test site at http://localhost:8889/

Some tests use Twenty Twenty-One and set some colors on the content by using various color pickers. Turns out the color palette on the test site is different from the color palette used on the development site. Thus, the tests are unreliable:

While developing, developers see the Twenty Twenty-One color palette.
The test site uses the default color palette, ignoring the one from Twenty Twenty-One.

Not to mention that contributors may not use wp-env. In that case, correctly updating the test snapshots is impossible because developers will get the correct color palette while the test site uses the wrong color palette.

For example, I use a standard wordpress-develop cloned repo with Gutenberg cloned within the plugins directory. I start my environment by using the WP local-env: npm run env:start. Everything works as expected. With this setup, Twenty-Twenty-One uses the correct color palette defined in the theme's functions.php file. Updating the test snapshots will update the colors used in the snapshots with the ones from the correct color palette.

These colors don't match the ones used on CI and the tests on CI will always fail.

When using wp-env, the dev site uses the correct color palette defined in the theme's functions.php file. However, turns out the test site uses the (wrong) default color palette defined in src/wp-includes/theme.json which makes very hard to build a reliable test and correctly update the snapshots.

Please consider that this unexpected difference between the dev and test environments has been a blocker for https://github.com/WordPress/gutenberg/pull/42187 for several weeks, as it was hard to figure out the reason for the test failure was actually something in the test environment.

Step-by-step reproduction instructions

Use wp-env.
On the dev site http://localhost:8888/, activate Twenty Twenty-One.
Edit a post.
Select a paragraph and from the block toolbar click: More > Highlight.
A popup with a color picker shows up.
Observe the color palette is the correct one from the Twenty Twenty-One functions.php:

Run the test that was failing on https://github.com/WordPress/gutenberg/pull/42187 in 'headed' mode:

npm run test:e2e -- --puppeteer-interactive --testPathPattern packages/e2e-tests/specs/editor/various/rich-text.test.js

Note: to make things faster, you may want to delete all tests from the test file except the test should preserve internal formatting.
While the test runs, observe the color palette is the wrong one from src/wp-includes/theme.json:

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

torounit commented 1 year ago

Color palette is disabled by this PR. https://github.com/WordPress/gutenberg/pull/18761

afercia commented 1 year ago

@torounit thanks for pointing me to #18761.

While I do understand the good intent behind that change, I'm not sure that's the right way to go.

I see some concerns were expressed on #18761 and I'm not sure they've been fully addressed.
The discussion on #18761 dates to the end of 2019 and maybe some things have changed over time in the expectations of what a test environment should provide to developers.
Lots of tests use activateTheme() to switch to a different theme, so the point of making the test environment 'theme agnostic' is a bit pointless.
No matter what, to me the dev and the test sites should be identical. The only goal of a test site is to run the tests on a different database so that all your posts on the dev site don't get deleted or trashed.
The dev and the test sites must be identical because developers will build tests based on what they actually see in the editor. E.g. I see that tabbing 3 times in the color picker picks up the white color. However, on the test site, that will pick up a different color. This is far from ideal.
Other differences / disabled features between the two sites may trick developers when they build tests.
Over the years, the WordPress leads and devs always tried to make their best to provide all contributors with a development / testing environment that runs reliably and can be set up in the easiest possible way. Contributors should not be required to use wp-env. They should be allowed to use whatever they want and the tests should run reliably whatever the actual setup is.

I'd liek to hear more thoughts and feedback from other developers. To me, #18761 appears to introduce more troubles than benefits and I'd tend to think it should be reverted.

Cc @youknowriad @epiqueras (not sure who else I can ping about this) 🙏

youknowriad commented 1 year ago

No matter whqt, to me the dev and the test sites should be identical. The only goal of a test site is to run the tests on a different database so that all your posts on the dev site don't get deleted or trashed.

I disagree with this take personally, test sites should have test data and could be very different from dev sites.

That said, today we have a better way of solving this. At the time that PR has been implemented, the tests used to rely on the default them of WordPress which was evolving. This is not the case anymore as we switched to a "fixed theme" explicitly when running the tests. So I think we can remove that test plugin and rely on the colors defined by the theme itself. We also have a dedicated test theme "emptytheme" that we can leverage to test some specific cases of palettes.

afercia commented 1 year ago

Thanks @youknowriad for your feedback. Removing the Normalize Theme would be good as I'd agree I'm not sure that halps developers in any way, today.

No matter whqt, to me the dev and the test sites should be identical. The only goal of a test site is to run the tests on a different database so that all your posts on the dev site don't get deleted or trashed.

I disagree with this take personally, test sites should have test data and could be very different from dev sites.

Yeah, let me rephrase it: they should be identical as in: identical configuration. Or, at least, the two sites configuration should be as similar as possible. Any difference should be clearly documented, otherwise contributors will likely spend hours trying to understand what's going on.

Of course, the actual data used on the test site may be crafted for specific use in the tests.

In addition to the above, I'd love to see some more consideration for the following points:

All the tests should run reliably also when installing Gutenberg as a plugin in a standard wordpress-develop install.
When running the tests on a standard wordpress-develop install. there's no test database so all your posts get deleted or trashed, which seems less than ideal.
When running the tests on a standard wordpress-develop install. there's no emptytheme available. For this and the previous point, I'd tend to think some effort should be made to improve the WP core local-env and add some configuration for a test site and hte emptytheme.

youknowriad commented 1 year ago

When running the tests on a standard wordpress-develop install. there's no emptytheme available. For this and the previous point, I'd tend to think some effort should be made to improve the WP core local-env and add some configuration for a test site and hte emptytheme.

Oh yeah, it's been some time no one actually worked on making sure we can run these tests properly on WordPress without the plugin. I did that at some point in the past but this effort kind of stopped because we never enabled it by default in Core tests. If it's mandatory in Core tests, people will pay more attention to the tests and flows and keep things running smoothly. But since it adds friction to core commits and PRs and due to some test instability at the time, I didn't make it mandatory.

noahtallen commented 1 year ago

I removed the env label, just because that's normally for development of the wp-env tool itself, but this behavior is unique to the Gutenberg plugin :)

WordPress / gutenberg