Comments Query Loop - Add e2e tests for the block #39502
Conversation
|
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
|
Right now all e2e tests are not working I guess due to Quick Edit and other JS broken on .org 😢 So we will wait for it to be fixed so we can keep working on them. |
df7a662 to
98529c2
Compare
|
The e2e test is currently failing with Looking at the test artifacts (available from the same page), there's a few screenshots that indicate that the block can't seem to be found in the inserter 🤔
I think this could be due to a non-FSE theme being active (I can repro that behavior locally). So I think we might need to activate an FSE theme for this to work first. The tests in this directory should provide some inspiration; we might also consider moving our new test there, since it's kinda related to the Site Editor 🤔 |
c2ed98b to
7a3ab6c
Compare
| // We won't select the option that we updated and will also remove some | ||
| // _transient options that seem to change at every update. |
There was a problem hiding this comment.
Stray comment? 😅
| // We won't select the option that we updated and will also remove some | |
| // _transient options that seem to change at every update. |
| it( 'Comment Query block insertion', async () => { | ||
| // We won't select the option that we updated and will also remove some | ||
| // _transient options that seem to change at every update. | ||
| await insertBlock( 'Comments Query Loop' ); | ||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | ||
| } ); |
There was a problem hiding this comment.
While it gives some confidence that the block can be inserted, I'm a bit skeptical if we should have a test that really only checks for that. Each test means additional maintenance (when the implementation changes, and the test has to be updated accordingly).
This is especially true with snapshot tests; while convenient, snapshot tests aren't very "semantic" in expressing our expectations. And it's quite possible that the exact block markup changes frequently (e.g. a newline is removed or introduced -- changes that wouldn't even have any impact on the block behavior), and there's a risk that we get used to updating them blindly and start ignoring the actual snapshot contents.
Another question would be why the Comments Query block needs a test to cover its insertion more than other blocks (since not all of them test for that). We've seen one case when the block was gone from the inserter (#39415), so that's a fair argument.
Still, TBH, I'd remove the snapshot test. IMO, it'd be best to have an e2e test that covers some behavior that's fairly specific to the block, and ideally has already seen a regression; a test like that would require block insertion anyway, so we'd get test coverage for that "for free". (I thought that we needed a test for the scenario described here, but it seems like you've covered that now with a unit test?)
There was a problem hiding this comment.
It is covered by a unit test, but, as you mention, is not so clear what is that test solving.
For that reason, maybe creating an e2e of this case scenario would be a better idea, the only cons is that we have to create a lot of new funcionalities:
- Setting and getting discussion options (almost done)
- Creating at least 3 comments on the post/page.
- Checking different elements on the frontend.
There was a problem hiding this comment.
I think that might be okay 🤔 Since this is covering a whole new area of Gutenberg (the Comments blocks), it makes sense that it requires us to introduce corresponding e2e helpers.
Setting and getting discussion options (almost done)
👍 🎉
Creating at least 3 comments on the post/page.
I guess we can start with a helper that creates one comment and allows to specify whether it's a reply to the post, or to a different comment.
We should probably ask ourselves whether we want to:
- Create those comments via the frontend, which will implicitly also test the frontend functionality of those blocks. It means that any e2e test relying on the util will break if any of those blocks break (probably a good thing), or change their behavior, in which case the util will need updating (potentially less good, since it means more maintenance); or
- Use the
wp-adminComments panel (i.e.wp-admin/edit-comments.php). This is going to be stabler as we don't expect it to change as much as Gutenberg and the new Comments blocks. But maybe we really want the "Create Comment" helper to work via the frontend, especially if we're adding more features that we want to cover? 🤔
Curious to hear your thoughts!
Checking different elements on the frontend.
There might be some precedent for this (but certainly not a lot), since it's a problem that doesn't just apply to the Comments blocks, but to FSE more generally. Maybe try looking in the Site Editor e2e tests directory.
There was a problem hiding this comment.
Use the wp-admin Comments panel (i.e. wp-admin/edit-comments.php). This is going to be stabler as we don't expect it to change as much as Gutenberg and the new Comments blocks. But maybe we really want the "Create Comment" helper to work via the frontend, especially if we're adding more features that we want to cover?
I'm afraid there is no option to create comments from there 😢 . So I guess we have to go with option 1 (creating on the front).
There was a problem hiding this comment.
I'm afraid there is no option to create comments from there 😢 . So I guess we have to go with option 1 (creating on the front).
Ah, oops 😅 Looks like I didn't pay close enough attention! Well, fewer decisions for us to make 😅
| await setOption( 'page_comments', true, 'options-discussion.php' ); | ||
| await setOption( 'comments_per_page', '2', 'options-discussion.php' ); |
There was a problem hiding this comment.
It seems like we're not really testing for any behavior based on these settings now?
There was a problem hiding this comment.
True, at least while the unit test is the one that checks the pagination behavior. Let's make an e2e to check the navigation links and these settings and functions will be necessary.
| await setOption( 'page_comments', false, 'options-discussion.php' ); | ||
| await setOption( 'comments_per_page', '50', 'options-discussion.php' ); |
There was a problem hiding this comment.
| it( 'can be created by typing "/comment query loop"', async () => { | ||
| // Create a list with the slash block shortcut. | ||
| await clickBlockAppender(); | ||
| await page.keyboard.type( '/comments query loop' ); | ||
| await page.waitForXPath( | ||
| `//*[contains(@class, "components-autocomplete__result") and contains(@class, "is-selected") and contains(text(), 'Comments Query Loop')]` | ||
| ); | ||
| await page.keyboard.press( 'Enter' ); | ||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | ||
| } ); |
There was a problem hiding this comment.
I'm afraid the same arguments as above apply 😕 It's a rather generic test that covers pretty standard block behavior -- the only reason it would fail that would be more or less specific to the Query Loop block is if it's not registered or otherwise broken. But as said above, we should cover block insertion as part of a test that covers a behavior that's more specific to the individual block (and we don't really need to cover different ways to insert the same block).
If we currently don't have any more block-specific tests (cause they're e.g. already covered by the unit test), I'd go as far as to suggest to remove them 😅
| * @param {string} value The value to set the option to. | ||
| * @param {string} adminPage The url of the admin page to visit. | ||
| * | ||
| * @return {Promise<string>} The current value of the option. |
There was a problem hiding this comment.
Looks like we're no longer returning any value from the helper function?
There was a problem hiding this comment.
Yes, we can add it if we ever need it, but yet I didn't find a case.
There was a problem hiding this comment.
Let's remove the JSDoc line it in case we don't actually return anything. I'd even lean towards removing it already so we don't forget about it; we can add it back later 🙂 YAGNI etc.
| * @param {string} value The value to set the option to. | ||
| * @param {string} setting The option, used to get the option by id. | ||
| * @param {string} value The value to set the option to. | ||
| * @param {string} adminPage The url of the admin page to visit. |
There was a problem hiding this comment.
Should we mention the default?
There was a problem hiding this comment.
I forgot about that one 😓
| $comment_query_max_num_pages = 5; | ||
| // We substract 1 because we created 1 comment at the beggining. | ||
| $post_comments_numbers = ( self::$per_page * $comment_query_max_num_pages ) - 1; | ||
| self::$comment_ids = self::factory()->comment->create_post_comments( |
There was a problem hiding this comment.
Seems like we're not using self::$comment_ids anywhere?
| $this->assertEquals( | ||
| build_comment_query_vars_from_block( $block ), | ||
| array( | ||
| 'orderby' => 'comment_date_gmt', | ||
| 'order' => 'ASC', | ||
| 'status' => 'approve', | ||
| 'no_found_rows' => false, | ||
| 'update_comment_meta_cache' => false, | ||
| 'post_id' => self::$custom_post->ID, | ||
| 'hierarchical' => 'threaded', | ||
| 'number' => self::$per_page, | ||
| 'paged' => $comment_query_max_num_pages, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Some of these seem circumstantial (and not really impacted by our choice of settings). Would it make sense to only test for some select attributes, rather than the entire array?
E.g.
$actual = build_comment_query_vars_from_block( $block );
$this->assertEquals( $actual[ 'paged' ], $comment_query_max_num_pages);
// Etc| /** | ||
| * Test that both "Older Comments" and "Newer Comments" are displayed in the correct order | ||
| * inside the Comment Query Loop when we enable pagination on Discussion Settings. | ||
| * In order to do that, it should exist a query var 'cpage' set with the $comment_args['paged'] value. | ||
| */ |
There was a problem hiding this comment.
TBH without knowing much about build_comment_query_vars_from_block, pagination settings, and the cpage query arg and paged value, this is a lot to swallow 😅 It's not obvious to me how all these things are related to the correct order of "Older Comments" and "Newer Comments".
The test may have its value in covering the underlying implementation to ensure the desired behavior, but maybe it'd actually make sense to still add the e2e test to cover that behavior?
There was a problem hiding this comment.
aea6688 to
3d12368
Compare
| } | ||
|
|
||
| // We check that there is a previous comments page link. | ||
| await page.waitForSelector( '.wp-block-comments-pagination-previous' ); |
There was a problem hiding this comment.
There's one minor issue with this: If the selector is not there, this will time out and throw an error -- i.e. we won't even make it to the assertion on the next line. This means that the assertion is kind of redundant.
So should we remove the assertion? No, since it's better to explicitly state our intent rather than implicitly.
Let's keep this as-is for now and merge it; I might file a follow-up to experiment with replacing the waitForSelector with waitForNavigation 🙂
There was a problem hiding this comment.
Tentative diff for later:
diff --git a/packages/e2e-tests/specs/experiments/blocks/comments-query.test.js b/packages/e2e-tests/specs/experiments/blocks/comments-query.test.js
index 55c6b4b07a..d3b8c880a9 100644
--- a/packages/e2e-tests/specs/experiments/blocks/comments-query.test.js
+++ b/packages/e2e-tests/specs/experiments/blocks/comments-query.test.js
@@ -48,12 +48,13 @@ describe( 'Comment Query Loop', () => {
`This is an automated comment - ${ i }`
);
await pressKeyTimes( 'Tab', 1 );
- await page.keyboard.press( 'Enter' );
- await page.waitForNavigation();
+ await Promise.all( [
+ page.keyboard.press( 'Enter' ),
+ page.waitForNavigation( { waitUntil: 'networkidle0' } ),
+ ] );
}
// We check that there is a previous comments page link.
- await page.waitForSelector( '.wp-block-comments-pagination-previous' );
expect(
await page.$( '.wp-block-comments-pagination-previous' )
).not.toBeNull();
@@ -61,21 +62,25 @@ describe( 'Comment Query Loop', () => {
await page.$( '.wp-block-comments-pagination-next' )
).toBeNull();
- await page.click( '.wp-block-comments-pagination-previous' );
+ await Promise.all( [
+ page.click( '.wp-block-comments-pagination-previous' ),
+ page.waitForNavigation( { waitUntil: 'networkidle0' } ),
+ ] );
// We check that there are a previous and a next link.
- await page.waitForSelector( '.wp-block-comments-pagination-previous' );
- await page.waitForSelector( '.wp-block-comments-pagination-next' );
expect(
await page.$( '.wp-block-comments-pagination-previous' )
).not.toBeNull();
expect(
await page.$( '.wp-block-comments-pagination-next' )
).not.toBeNull();
- await page.click( '.wp-block-comments-pagination-previous' );
+
+ await Promise.all( [
+ page.click( '.wp-block-comments-pagination-previous' ),
+ page.waitForNavigation( { waitUntil: 'networkidle0' } ),
+ ] );
// We check that there is only have a next link
- await page.waitForSelector( '.wp-block-comments-pagination-next' );
expect(
await page.$( '.wp-block-comments-pagination-previous' )
).toBeNull();
There was a problem hiding this comment.
Thanks Carlos! LGTM! 🚢
Here's one note for a follow-up PR 🙂
BTW I just learned the other day that we now have an alternative way of writing e2e tests (using playwright, and generally using the REST API rather than wp-admin to change settings). We can give that a try some time, it might speed up e.g. comment creation.
PR draft: #39826 |
|
Can you have a look at the test failure on https://github.com/WordPress/gutenberg/runs/6021185292?check_suite_focus=true
Those blocks are no longer experimental, so the location of the test file should also get updated. |
|
Since that test was re-run successfully, there are no test artifacts available from that test run. However, this one has the same error (and wasn't re-run), so there are artifacts:
Looks like all comments are rendered on the same page, which explains why there are no pagination links shown. This means that setting the relevant comments pagination options doesn't seem to have worked in this test run 😕 |
|
I'm not quite sure why this is happening 😕 After all, Or maybe the page hasn't fully loaded when we try to focus the relevant field and change it? 🤔 |
I created a PR to address:
|
What?
We are adding e2e and unit tests for the Comments Query Loop block.
Why
To prevent future changes that could make the pagination comments link bug happen again. Also to prevent code that would break the Comments Query Loop block insertion.
How
We are adding the block using both the inserter and the shortcode
/comments query loop.Also, we fixed a bug on #39227 where, as the
cpagequery var was not defined in a specific case, the "Older Comments" and "Newer Comments" were not working. This PR also adds unit testing for those lines of code.