Comment Template Block: Add test coverage for context setting and inner block insertion #4507
Conversation
|
Note that the |
ockham
force-pushed
the
add/test-coverage-for-comments-template-block-context
branch
from
af0ffd9
to
ddf8439
Compare
|
Added tests from WordPress/gutenberg#50883. |
ockham
changed the title
|
Hi 👋 |
|
@tellthemachines Apologies, I still haven't gotten around to do this. Upon second thought, I think I should split the PR in two, one of which we should be able to merge before Beta 1 (when blocks are synced from GB), whereas the other test will only pass after that sync. I'll try to take care of that splitting, plus filing the required tickets, early next week. (Then again, none of this is super-urgent, so it's not the end of the world if it doesn't go into 6.3.) |
|
Thanks for the update @ockham ! |
ockham
force-pushed
the
add/test-coverage-for-comments-template-block-context
branch
from
9c1457c
to
c9485df
Compare
Rebased to pick up the updated packages and to make tests pass 🤞 Wondering if we should consider including these with 6.3 after all, given https://core.trac.wordpress.org/ticket/58699. (The tests wouldn't have caught that, but they provide some confidence that we don't introduce any regressions when fixing that. This might be a bit more academic here, since the fix has to be made in GB anyway, where those tests are already in place.) |
|
I'm not against adding more test coverage 😄 Is this ready for review then? Asking because it's still in draft mode. |
Ah yep, I guess it is -- apologies 😅 I guess I wasn't 100% sure if the tests were "minimal" enough (especially |
ockham
force-pushed
the
add/test-coverage-for-comments-template-block-context
branch
from
c9485df
to
5cc94a3
Compare
There was a problem hiding this comment.
These tests are generally looking good to add to me. Just left a question about the remove_filter and add_filter calls for Gutenberg filters. I imagine those can be removed? I couldn't find gutenberg_auto_insert_child_block in the Gutenberg repo, but I might be missing some context there potentially.
Ah, great spot! Yep, they can be removed; I'll take care of that. Thank you for flagging 👍 😄 |
| return $inserted_content . $block_content; | ||
| }; | ||
|
|
||
| add_filter( 'render_block', $render_block_callback, 10, 3 ); |
There was a problem hiding this comment.
No need to store the callback, just pass it directly here. The test suite cleans up all filters after each test, so no need to call remove_filter()
There was a problem hiding this comment.
Oh, you mean like inline? Cool, will do 👍
|
Took me a while, but I’ve filed the ticket. I've also addressed everyone's feedback (thank you!) We're pretty close to RC1, so I'm not sure if folks are comfortable merging this so late in the game. OTOH, it's a test-only change that's supposed to guard against regressions 🤔 |
Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
ockham
force-pushed
the
add/test-coverage-for-comments-template-block-context
branch
from
756caa6
to
ec9a7ae
Compare
…_context filter at priority 2
|
Added some more coverage in d3e3ffd to cover the scenario fixed by WordPress/gutenberg#52364. cc/ @dlh01 |
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
Just some nit feedback, but with these and @mukeshpanchal27's suggestions, this looks good to go. Thanks @ockham!
Pre-approving so as not to hold up commit as my internet is tempramental today.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>
|
Thanks all! Committed to Core in https://core.trac.wordpress.org/changeset/56262 😊 |
Backport of WordPress/gutenberg#50879 and WordPress/gutenberg#50883.
Trac ticket: https://core.trac.wordpress.org/ticket/58839
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.