Block Bindings: Add block bindings e2e tests #58550
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
requested review from
gziolo,
artemiomorales and
michalczaplinski
SantosGuillamot
added
[Type] Automated Testing
|
Size Change: -9.85 kB (-1%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Feb 1, 2024
gziolo
reviewed
Feb 1, 2024
gziolo
reviewed
Feb 1, 2024
gziolo
reviewed
Feb 1, 2024
gziolo
reviewed
Feb 1, 2024
|
This looks great! There is so much coverage for the existing functionality. I can't think at the moment about any more use cases. We probably can even remove some tests that check the conditions that never change between the post and template contexts, or we could explore ways to reuse the tests but executed them with a different editor loaded. @kevin940726 is also a great person to help structure the code. |
|
I've reduce the number of tests so the ones that were repeated are only part of the Template context group. |
gziolo
approved these changes
Feb 2, 2024
There was a problem hiding this comment.
The only remaining task from my perspective would be confirming how to test the Image with a custom URL. At the moment, the URL points to an external domain, so maybe we could use the one that the Image uses for previews:
An alternative would be uploading one of the images in the test/e2e/assets folder and setting it as the default for the custom fields created with REST API. I hope we can go with the more straightforward approach.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
In the latest commit, I made it work by uploading the images in assets. I've also changed the structure of the tests a bit. It follows this structure now:
It is important to note that, depending on the level, I'm doing different Block bindings groupBefore All
Before Each After Each
After All
Template context groupBefore Each
Post/page context groupBefore Each
Image > Post/page context groupI'm doing this here because it is only used in this case and we avoid uploading an unnecessary image. Before All
Before Each
Hope that makes sense. Let me know what you think. I've updated the opening post to reflect this. |
gziolo
reviewed
Feb 2, 2024
gziolo
reviewed
Feb 2, 2024
SantosGuillamot
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
kevin940726
reviewed
Feb 4, 2024
Comment on lines
+236
to
+239
| const paragraphContent = await paragraphBlock.textContent(); | ||
| expect( paragraphContent ).toBe( | ||
| variables.customFields.textKey | ||
| ); |
There was a problem hiding this comment.
There's a handy toHaveText assertion ;)
There was a problem hiding this comment.
I've used toHaveText in this commit as part of a follow-up pull request: link.
| // Alignment controls exist. | ||
| await expect( | ||
| page.getByRole( 'button', { | ||
| name: variables.labels.align, |
There was a problem hiding this comment.
Nit: I normally prefer inlining these variables and primitives to make the tests more readable. Also, we can nest locators to make it clearer that it's a control in the block settings.
Comment on lines
+267
to
+269
| const isContentEditable = | ||
| await paragraphBlock.getAttribute( 'contenteditable' ); | ||
| expect( isContentEditable ).toBe( 'false' ); |
There was a problem hiding this comment.
We have toHaveAttribute and also toBeEditable.
There was a problem hiding this comment.
I've used toHaveAttribute this commit as part of a follow-up pull request: link.
I tried using toBeEditable, but it seems it is always true even if contenteditable is set to false.
| test( 'Should show the value of the custom field', async ( { | ||
| editor, | ||
| } ) => { | ||
| await editor.insertBlock( variables.blocks.paragraph ); |
There was a problem hiding this comment.
Nit: IMHO, I think these "variables" are less readable because I have to scroll up and down this file to know exactly what it is. I would probably just inline this, or make a function if it's used several times. These are tests, so I wouldn't worry too much about repetitions.
There was a problem hiding this comment.
I was more worried about the maintenance of the tests. Anyway, I addressed that in this commit as part of a follow-up pull request: link.
This was referenced Feb 5, 2024
|
Thanks a lot for the feedback @kevin940726 ! 🙂 I've included your suggestions as part of this pull request. I'm also adding some checks in the frontend. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adding e2e tests for the editor experience of the block bindings. These are the tests I've added so far:
Template context
1 - Paragraph
When the content is connected:
2 - Heading
When the content is connected:
3 - Button
When only the text is connected:
When only the URL is connected:
When both are connected:
4 - Image
When non-bindings are defined:
When only the URL is bound:
When only the ALT is bound:
When only the TITLE is bound:
When UTL and ALT are connected, but not the TITLE:
Post/page context
1 - Paragraph
When the content is connected:
2 - Heading
When the content is connected:
3 - Button
When only the text is connected:
4 - Image
When only the URL is bound:
When only the ALT is bound:
When only the TITLE is bound:
When UTL and ALT are connected, but not the TITLE:
Why?
Right now, they don't exist, and the testing has to be done manually.
How?
Using the existing e2e mechanisms. I had to create a PHP plugin to register a couple of custom fields to be used.
The tests follow this structure:
It is important to note that, depending on the level, I'm doing different
beforeAll, beforeEach,afterEach,afterAll`. Let me try to summarize that:Block bindings group
Before All
emptythemegutenberg-test-block-bindingsplugin, which register the custom fields.Before Each
Nothing
After Each
After All
Template context group
Before Each
Post/page context group
Before Each
Image > Post/page context group
I'm doing this here because it is only used in this case and we avoid uploading an unnecessary image.
Before All
Before Each
postId.url_custom_fieldthrough the REST API.Hope that makes sense. Let me know what you think.