Gutenberg: E2E Tests: Admin - 4 Test Suite fails

[ockham]

Describe the bug
On all PRs filed for branches that have been branched off master recently, the Admin - 4 Test Suite fails because of a snapshot test (e.g.).

To reproduce
See above

Expected behavior
The test should pass

Additional context
The reason for this is probably #23848, which updated button labels in the 'Two Buttons' block pattern (Button One became Get started, Button Two became Learn more.). While it did update the snapshot also, pattern registration doesn't actually register Gutenberg's own 'Two Buttons' block pattern, as its already present in Core -- with the 'old' strings.

I've set priority to High, as this is causing part of the e2e tests to fail for everyone.

cc/ @enriquesanchez @kjellr @mapk @youknowriad

[youknowriad]

This was going to happen at some point :). We need to update the code that register the block patterns on Gutenberg to "replace" the core patterns.

[ockham]

I'll file a PR :+1:

[ockham]

This was going to happen at some point :). We need to update the code that register the block patterns on Gutenberg to "replace" the core patterns.

Since the registration code still says This can be removed when plugin support requires WordPress 5.5.0+., I'm wondering what our strategy is for that? Do we want to eventually get rid of the Gutenberg overrides? I think we can, if we sync back the changes to Core.

But the underlying problem would remain -- if we have (snapshot) tests in Gutenberg, we should really be testing the behavior of Gutenberg code (rather than 'external' code). :thinking:

[youknowriad]

I don't think we should remove the registration yet. What we should do is something like (per pattern):

if ( isRegistered( pattern ) {
  unregister( pattern)
}
register( pattern );

Needs to run on "init" after Core's registration.

[ockham]

I don't think we should remove the registration yet. What we should do is something like (per pattern):

if ( isRegistered( pattern ) {
  unregister( pattern)
}
register( pattern );

Needs to run on "init" after Core's registration.

Yeah, that was my plan for the short-term fix anyway. My question was about the longer-term strategy :slightly_smiling_face:

[youknowriad]

It's not clear yet. Potentially this short term strategy will remain forever if we continue to iterate on the patterns.
If we stop, we'd have to remove all the patterns from Gutenberg or at least the ones that don't get updated.

[ockham]

Fix in #24004.

It's not clear yet. Potentially this short term strategy will remain forever if we continue to iterate on the patterns.
If we stop, we'd have to remove all the patterns from Gutenberg or at least the ones that don't get updated.

Wondering a bit about the ramifications of this. We have overrides in a lot of places -- block pattern _categories_ are one very obvious example (since they're right below in the same file), but we also extend REST API endpoints etc. It'd be a bit unappealing to carry all those modifications along :thinking:

OTOH, if we drop them from Gutenberg, people will have to modify them in Core. Not only has this DevEx implications; the bigger problem that I see is that we'd still have tests in Gutenberg that test for some behavior that's not defined within Gutenberg. And not all of these things can be tested in Core -- e.g. we can't test block pattern insertion in Core, when it's implemented in Gutenberg.

Maybe we could modify tests to really only test whatever is implemented in the same repo? I.e. test that block insertion _works_ in Gutenberg, but test block pattern UI strings in Core?

[youknowriad]

OTOH, if we drop them from Gutenberg, people will have to modify them in Core. Not only has this DevEx implications; the bigger problem that I see is that we'd still have tests in Gutenberg that test for some behavior that's not defined within Gutenberg. And not all of these things can be tested in Core -- e.g. we can't test block pattern insertion in Core, when it's implemented in Gutenberg.

I'm not concerned about the devx implication on Core. Working on trac + Github for reviews is as easy as Working on Gutenberg tbh. And for the tests, you're right but we already rely on wp-admin (implemented on Core) so I don't see it as different.

And for the overrides, that's fine because we want to support Core versions that don't have these features yet but we do regularly remove things when the minimum supported version of WordPress is changed.