e2e tests for post comments form placeholder #40396
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
ribaricplusplus
requested review from
ntwb,
nerrad and
ajitbohra
as code owners
ribaricplusplus
added
the
[Block] Post Comments Form
|
Size Change: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Really nice work @ribaricplusplus 👍 I left some comments for you below
| ); | ||
|
|
||
| // Compare the structures | ||
| expect( editorBlockStructure ).toEqual( frontendBlockStructure ); |
There was a problem hiding this comment.
I think we could improve this part a bit. I think that comparing the whole structure here is not the best pattern:
- If there is a single error inside of that assertion, the whole assertion fails without giving the information about which particular of the 3 selectors failed. (PS. I haven't tested that this is the case, perhaps playwright is smart about it, but I'm not so intimately familiar with it)
- In general, we should not assume that the structure of the frontend and the editor is going to be the same in any way. It just so happens incidentally to be the case here but it might change in the future.
- Simple selectors are more readable and because this is test code we probably should not build abstractions too quickly. Without them it's easier to change or delete tests.
Could we turn this getStructureFromHandle abstraction into a list of simpler selectors + specific assertions about what is expected instead of comparing it with the content in the frontend? So something like:
await expect(page.$('.wp-block-post-comments-form .comment-form input[type="submit"]').not.toBeNull();
|
Thank you for taking a look!
Oh, alright - I incorrectly assumed otherwise. In that case, i don't think there is much benefit to having the second test at all so I removed it. |
|
All right, let's merge it 👍 Thanks a lot for helping out @ribaricplusplus ! |
ockham
pushed a commit
that referenced
this pull request
Apr 21, 2022
* Tests * Change tests directory * Remove second test * Fix test
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.
Added some e2e tests for #40368.
@michalczaplinski could we merge this into your PR?