Block Bindings: Add tests for the frontend and polish the existing ones #58676
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
added
[Type] Automated Testing
SantosGuillamot
requested review from
gziolo,
artemiomorales and
michalczaplinski
SantosGuillamot
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
|
Size Change: +7 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Feb 5, 2024
| @@ -89,4 +90,6 @@ export class Editor { | |||
| switchToLegacyCanvas.bind( this ); | |||
| /** @borrows transformBlockTo as this.transformBlockTo */ | |||
| transformBlockTo: typeof transformBlockTo = transformBlockTo.bind( this ); | |||
| /** @borrows updatePost as this.updatePost */ | |||
| updatePost: typeof updatePost = updatePost.bind( this ); | |||
There was a problem hiding this comment.
It is recommended to avoid overloading the end-to-end test utilities.
Let's make it a test-specific helper. Example - https://github.com/WordPress/gutenberg/blob/trunk/test/e2e/specs/editor/various/list-view.spec.js#L1052.
There was a problem hiding this comment.
Great 🙂 I just did that in this commit.
There was a problem hiding this comment.
TBH, I think updatePost should be an util because it's used so many times in different test suites 😅. Same as how publishPost and saveDraft are utils. This is not related to this PR though.
There was a problem hiding this comment.
I think the proper name for this utility would be updatePublishedPost() (it would fail if the post is draft). The posts have different update flows based on their status. We should account for them in updatePost or use more explicit names.
There was a problem hiding this comment.
Possibly we also want to consider saving multiple entities and different post types in the site editor as well. So I agree that this can be thought through in another PR/issue. 👍
|
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. |
Mamaduka
reviewed
Feb 5, 2024
| * @param this | ||
| */ | ||
| export async function updatePost( this: Editor ) { | ||
| await this.page.click( 'role=button[name="Update"i]' ); |
There was a problem hiding this comment.
It would be a good idea to narrow down the locator. The "update" is a common label.
await this.page
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'button', { name: 'Update' } )
.click();There was a problem hiding this comment.
Totalle makes sense. I just did that in this commit.
Mamaduka
approved these changes
Feb 5, 2024
michalczaplinski
approved these changes
Feb 5, 2024
kevin940726
approved these changes
Feb 6, 2024
| @@ -229,133 +50,230 @@ test.describe( 'Block bindings', () => { | |||
| test( 'Should show the value of the custom field', async ( { | |||
There was a problem hiding this comment.
Nit: A lot of these tests seem duplicated. We can simply use a for loop to cycle through each block (paragraph, heading, image, etc) to accomplish the same thing with less code. However, I think maybe we only need to test one block since all of them share the same code. The others can be tested in a much faster way like unit tests or integration tests. This also means we don't have to add more tests as we support more blocks in the future. These tests increase the coverage but not necessarily increase the confidence in our code, IMHO.
I don't think it matters though 😅, and we can always refine our tests. It's just an open discussion that we can circle back in the future.
There was a problem hiding this comment.
I agree that we could possibly reduce the tests. For this initial version, I just wanted to ensure that the initial set of blocks work as expected. Anyway, I believe it's worth considering it for a follow-up PR, maybe when we add support for more blocks?
There was a problem hiding this comment.
As long as the handling is hardcoded, it's better to have everything covered with detailed e2e tests. In particular, it's important for the frontend where we still can't fully leverage HTML API to replace attributes in the saved HTML. At some point, we will be able to follow what @kevin940726 recommended.
| name: 'Block: Button', | ||
| exact: true, | ||
| } ) | ||
| .locator( 'div' ); |
There was a problem hiding this comment.
Do you know why we need this div locator?
There was a problem hiding this comment.
In the editor, the button block are two nested divs:
<div aria-label="Block: Button">
<div role="textbox" contenteditable="false"></div>
</div>
As I wanted to check if contenteditable is true/false, I needed to access the inner div.
There was a problem hiding this comment.
Could we instead access it by its role? getByRole( 'textbox' )
There was a problem hiding this comment.
I made this change as part of this other pull request.
Comment on lines
+273
to
+276
| await expect( buttonBlock ).toHaveAttribute( | ||
| 'contenteditable', | ||
| 'false' | ||
| ); |
There was a problem hiding this comment.
Would not.toBeEditable() work here?
There was a problem hiding this comment.
I tested that, but toBeEditable always return true even if contenteditable is false. I don't know what is the issue.
| } | ||
|
|
||
| // Helper to add an anchor/id to be able to locate the block in the frontend. | ||
| async setId( testId ) { |
There was a problem hiding this comment.
Why don't we just set the id when we insert the block?
There was a problem hiding this comment.
How can we do that? Can we add an id that persists in the frontend?
There was a problem hiding this comment.
I believe this is possible by adding the anchor attribute?
await editor.insertBlock( {
name: 'core/paragraph',
attributes: {
anchor: 'paragraph-id',
content: 'My paragraph
},
} );There was a problem hiding this comment.
I made this change as part of this other pull request.
|
Thanks a lot for the reviews 🙂 I'm merging the PR now, and I can work on follow-up PRs if needed due to the ongoing discussions. |
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?
As a follow-up of the original block bindings tests, I started this pull request by adding a couple of steps more in the tests to check that everything works as expected once the post is published.
In the end, I took the opportunity to address the feedback received after merging the original pull request.
It covers, divided by commits:
toHaveTextassertion: link.toHaveAttributeassertion: link.Why?
Testing the frontend will provide security and will let us iterate faster.
Additionally, I addressed the feedback from @kevin940726 to polish the code.
How?
There are a couple of important aspects I am not sure about:
updatePosthelper to be able to see the content in the frontend: link.