Replace TT1-Blocks with Empty Theme in the wp-env of gutenberg and CI#37446
Conversation
youknowriad
added
[Type] Build Tooling
| "core": "WordPress/WordPress", | ||
| "plugins": [ "." ], | ||
| "themes": [ "WordPress/theme-experiments/tt1-blocks#tt1-blocks@0.4.7" ], | ||
| "themes": [ "WordPress/theme-experiments/emptytheme#emptytheme#8f5e564b366db75e6067244ef54b91d06d4f8d8c" ], |
There was a problem hiding this comment.
We could probably use a new tag from that repo.
There was a problem hiding this comment.
Creating a tag sounds like a good idea, probably after porting changes that are needed to the theme.
|
I like this idea a lot, Empty Theme is an excellent tool to verify whether issues come from theme or editor styles, or whether they come from upstream. So much in fact that it'd make sense to include "Test in Empty Theme" as a suggestion in the PR review instructions. That being said, I'd love to see some tiny tweaks to the Empty Theme directly to help test some of the theme.json properties, for example "Font Size" is not accessible there: |
|
Size Change: -3.47 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
100% agree - assuming that the Empty Theme keeps its configuration the same forever. In the past we had to patch every theme or tests so they continue passing after modification were applied. This looks like a way to resolve all those issues. |
|
Would it make sense to make the test theme part of this repository? |
Definitely something we can consider. What do you all think? @WordPress/gutenberg-core |
|
+1 from me. Makes sense to have the empty-theme here if we use it for tests etc. |
|
It makes sense to me as well. We're already keeping plugins required for tests. A small bonus will be that symlinking is faster than pulling repo from Github. |
|
Yeah, I think making it part of the repo here would be totally fine. 👍
Oh right, good catch. This PR should fix it: WordPress/theme-experiments#295 |
|
Would definitely love to have whatever theme we use to test FSE (and default to for wp-env) included in this repo. Empty Theme sounds like a good (and lightweight) choice. |
|
This is currently failing with We had a similar error before with TT1 Blocks, since apparently our git library doesn't understand SHAs. How about we fix this for now by tagging |
Sure thing, here you go! https://github.com/WordPress/theme-experiments/releases/tag/emptytheme%401.0.0 |
youknowriad
force-pushed
the
update/replace-tt1-blocks-with-emptytheme
branch
from
460f4a3 to
ef6d463
Compare
|
Can you please include testing instructions? :) |
|
@carolinan Sure I added some instructions but it's mostly about the e2e tests passing. |
|
I'm potentially wrong here, I was switching branches :P |
| await revertTemplate(); | ||
| await save(); | ||
| await undoRevertInHeaderToolbar(); | ||
| // there's a bug which probably also exists where the redo button stays disabled. |
There was a problem hiding this comment.
I skipped this test because there's an undo/redo bug that is only visible with the empty theme for some reason but not with the tt1-blocks theme.
cc @ntsekouras if you have any ideas here.
There was a problem hiding this comment.
These bug are so fun 😢 . I've not found it yet, but I'll keep looking. It's so weird that it behaves differently on themes..
There was a problem hiding this comment.
I think it's related to this #35892 (comment)
youknowriad
force-pushed
the
update/replace-tt1-blocks-with-emptytheme
branch
from
223fe34 to
e733234
Compare
| } ); | ||
|
|
||
| it( 'allows pages to be created from the navigation block and their links added to menu', async () => { | ||
| it.skip( 'allows pages to be created from the navigation block and their links added to menu', async () => { |
There was a problem hiding this comment.
@talldan As seen on the latest commits on trunk, this test is failing too often, I'm skipping it for now.
There was a problem hiding this comment.
It's been failing for a while now. I suggested skipping it in slack last week.
There was a problem hiding this comment.
Apologies, I did get some failures on this when working on it, but I thought I'd solved them all as they passed multiple times locally.
Some discussion on fixing the test - #37755.
|
The performance tests here will fail because the "emptytheme" is not on trunk, I think we should probably just ignore that failure and merge. |
Does this mean that the "Compare performance with current WordPress Core and previous Gutenberg versions" check will fail as well once we merge this into the trunk? |
Good question, potentially yes, I wonder if we should limit it to the post editor for now in trunk? |
|
Or alternatively, we could decide to use the ".wp-env` file from the "perf tests branch" instead of the one from the plugin's branch. I guess this won't work because the empty theme folder doesn't exist in the old versions. |
|
Sorry, I don't have a solution right now. I was just thinking out loud 😄 |
youknowriad
force-pushed
the
update/replace-tt1-blocks-with-emptytheme
branch
from
c5a052e to
695d793
Compare
youknowriad
force-pushed
the
update/replace-tt1-blocks-with-emptytheme
branch
from
90ff960 to
5e5ce51
Compare
|
I've reworked the performance job (and added some docs about how it works) but it seems it's still failing from time to time on the site editor thought 🤔 (and not specifically on trunk) |
|
Ok I've managed to make the performance tests (document them and improve how they work). We should be good to go here. |
|
It looks like Performance tests are failing on the trunk after this got merged. |
11 participants
At this point, TT1-Blocks is starting to be outdated, we should consider removing it from the Gutenberg dev env and choosing a better theme for development.
We also now have 2022 that is present in the default environment now that it's merged in WordPress trunk.
That said, I decided to replace tt1-blocks with emptytheme here because I think it's the best theme for development, it doesn't have theme styles that can impact behavior of blocks... It's also unopinonated about everything. We still have 2022 in the environment if we want to work with a styled theme but for CI, emptytheme is probably a better default.
WDYT?
I also noticed that in our docs, we still recommend tt1-blocks to test FSE, we should probably update this recommendation separately.
testing instructions
npm run wp-env destroy && npm run wp-env startAppearance.