-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Tests: Ensure whitespace appears in assertEqualHTML tests #10765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Tests: Ensure whitespace appears in assertEqualHTML tests #10765
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
I've pushed commit This is a good example for discussion, I'd like to hear other opinions: @ockham @dmsnell |
| case '#text': | ||
| $text_content = $processor->get_modifiable_text(); | ||
| if ( '' === trim( $text_content, " \f\t\r\n" ) ) { | ||
| if ( '' === $text_content ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the main problem, it effectively eliminated whitespace text completely, and leading whitespace from existing text nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added in the first place? The trim() here occurs after decoding, meaning that any raw markup would have already been processed and whitespace normalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was introduced as part of block delimiters appearing in the HTML tree. That change is not open source, this was largely developed in another project before being proposed for Core.
|
I was surprised when whitespace wasn't detected recently but didn't think to look further. This explains it and the changes in the scripts tests are important. |
|
If we consider block delimiters as analogous to HTML tags, then it makes complete sense to preserve their inner whitespace. I'm more and more convinced that it should be preserved. |
| $expected = "<script src='/main-script-d4.js' id='main-script-d4-js' data-wp-strategy='defer'></script>"; | ||
| $this->assertEqualHTMLScriptTagById( $expected, $output, 'Scripts registered as defer but that have all dependents with no strategy, should become blocking (no strategy).' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may seem surprising that this assertion removes a trailing newline. This is using assertEqualHTMLScriptTagById() which matches the script tag, so a trailing newline will never be expected.
assertEqualHTMLScriptTagById() could probably trim its input, but it seems fine to just clean things up.
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @gclapps0612-cmd. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…ages The diff in the HTML is already presented by these tests. Printing again just introduces more noise.
assertEqualHTML already displays HTML differences
This reverts commit 23e18f7.
|
Commit It's a bit surprising because HTML is being compared with rendered block markup. This includes odd whitespace combinations like diff --git a/tests/phpunit/tests/blocks/wpBlock.php b/tests/phpunit/tests/blocks/wpBlock.php
index b3c11ad084386..85fa6e7151d46 100644
--- a/tests/phpunit/tests/blocks/wpBlock.php
+++ b/tests/phpunit/tests/blocks/wpBlock.php
@@ -387,13 +387,20 @@ public function data_provider_test_render_enqueues_scripts_and_styles(): array {
'all_printed' => array(
'set_up' => null,
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic">Hello World!</p>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <div class="static-child">First child</div>
+	
+ <p class="dynamic">Hello World!</p>
+	
+ <div class="static-child">Last child</div>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -414,13 +421,20 @@ static function ( $content ) {
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic filtered">Hello World!</p>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <div class="static-child">First child</div>
+	
+ <p class="dynamic filtered">Hello World!</p>
+	
+ <div class="static-child">Last child</div>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'dynamic-extra', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -430,12 +444,20 @@ static function ( $content ) {
add_filter( 'render_block_core/dynamic', '__return_empty_string' );
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <div class="static-child">First child</div>
+	
+	
+	
+ <div class="static-child">Last child</div>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module' ),
@@ -456,12 +478,20 @@ static function ( $enqueue, $block_name ) {
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <div class="static-child">First child</div>
+	
+	
+	
+ <div class="static-child">Last child</div>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -488,11 +518,16 @@ static function ( $content ) {
add_filter( 'render_block_core/static-child', '__return_empty_string' );
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <p class="dynamic">Hello World!</p>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <p class="dynamic">Hello World!</p>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'dynamic-view-script-module' ),
@@ -512,12 +547,18 @@ static function ( $content ) {
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic">Hello World!</p>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+	
+ <div class="static-child">First child</div>
+	
+ <p class="dynamic">Hello World!</p>
+	
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),A simple alternative in this case is to remove the tabs from the block markup in the test. |
|
Here's a similar diff of the test fix without the block indentation (commit diff --git a/tests/phpunit/tests/blocks/wpBlock.php b/tests/phpunit/tests/blocks/wpBlock.php
index b3c11ad084..ae067e7d79 100644
--- a/tests/phpunit/tests/blocks/wpBlock.php
+++ b/tests/phpunit/tests/blocks/wpBlock.php
@@ -371,13 +371,13 @@ class Tests_Blocks_wpBlock extends WP_UnitTestCase {
$block_markup = <<<'HTML'
<!-- wp:static -->
<div class="static">
- <!-- wp:static-child -->
- <div class="static-child">First child</div>
- <!-- /wp:static-child -->
- <!-- wp:dynamic /-->
- <!-- wp:static-child -->
- <div class="static-child">Last child</div>
- <!-- /wp:static-child -->
+<!-- wp:static-child -->
+<div class="static-child">First child</div>
+<!-- /wp:static-child -->
+<!-- wp:dynamic /-->
+<!-- wp:static-child -->
+<div class="static-child">Last child</div>
+<!-- /wp:static-child -->
</div>
<!-- /wp:static -->
HTML;
@@ -387,13 +387,20 @@ HTML;
'all_printed' => array(
'set_up' => null,
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic">Hello World!</p>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<div class="static-child">First child</div>
+
+<p class="dynamic">Hello World!</p>
+
+<div class="static-child">Last child</div>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -414,13 +421,20 @@ HTML;
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic filtered">Hello World!</p>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<div class="static-child">First child</div>
+
+<p class="dynamic filtered">Hello World!</p>
+
+<div class="static-child">Last child</div>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'dynamic-extra', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -430,12 +444,20 @@ HTML;
add_filter( 'render_block_core/dynamic', '__return_empty_string' );
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<div class="static-child">First child</div>
+
+
+
+<div class="static-child">Last child</div>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module' ),
@@ -456,12 +478,20 @@ HTML;
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <div class="static-child">Last child</div>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<div class="static-child">First child</div>
+
+
+
+<div class="static-child">Last child</div>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ),
@@ -488,11 +518,16 @@ HTML;
add_filter( 'render_block_core/static-child', '__return_empty_string' );
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <p class="dynamic">Hello World!</p>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<p class="dynamic">Hello World!</p>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'dynamic-view-script-module' ),
@@ -512,12 +547,18 @@ HTML;
);
},
'block_markup' => $block_markup,
- 'expected_rendered_block' => '
- <div class="static">
- <div class="static-child">First child</div>
- <p class="dynamic">Hello World!</p>
- </div>
- ',
+ 'expected_rendered_block' => <<<'HTML'
+
+<div class="static">
+
+<div class="static-child">First child</div>
+
+<p class="dynamic">Hello World!</p>
+
+</div>
+
+HTML
+ ,
'expected_styles' => array( 'static-view-style', 'static-child-view-style', 'dynamic-view-style' ),
'expected_scripts' => array( 'static-view-script', 'static-child-view-script', 'dynamic-view-script' ),
'expected_script_modules' => array( 'static-view-script-module', 'static-child-view-script-module', 'dynamic-view-script-module' ), |
| case '#text': | ||
| $text_content = $processor->get_modifiable_text(); | ||
| if ( '' === trim( $text_content, " \f\t\r\n" ) ) { | ||
| if ( '' === $text_content ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added in the first place? The trim() here occurs after decoding, meaning that any raw markup would have already been processed and whitespace normalized.
| 'expected_rendered_block' => <<<'HTML' | ||
| <div class="static"> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure how to think through this, and I think you are alluding to it in how we split blocks from the HTML, but is this right? It seems to be producing two newlines which I wouldn’t have expected.
Also, the test code itself performs an initial trim() on the supplied test data. Could that be making these more confusing than they need to be. That @todo looks like a good candidate for WP_Block_Processor::extract_full_block_and_advance()
// test_render_enqueues_scripts_and_styles:L670
// TODO: Why not use do_blocks() instead?
$parsed_blocks = parse_blocks( trim( $block_markup ) );There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure how to think through this, and I think you are alluding to it in how we split blocks from the HTML, but is this right? It seems to be producing two newlines which I wouldn’t have expected.
The block markup is this (after removing leading whitespace in this PR):
<!-- wp:static -->
<div class="static">
<!-- wp:static-child -->
<div class="static-child">First child</div>
<!-- /wp:static-child -->
<!-- wp:dynamic /-->
<!-- wp:static-child -->
<div class="static-child">Last child</div>
<!-- /wp:static-child -->
</div>
<!-- /wp:static -->Rendering removes the block delimiters (and replaces the registered dynamic block), but preserves everything else. The result is this:
<div class="static">
<div class="static-child">First child</div>
<p class="dynamic">Hello World!</p>
<div class="static-child">Last child</div>
</div>Before removing the indentation, this is what caused the \n\t\n sequences (whitespace characters added for clarity):
<!-- wp:static -->␊
␉ <div class="static">␊
␉ <!-- wp:static-child -->␊
␉ <div class="static-child">First child</div>␊
␉ <!-- /wp:static-child -->␊
␉ <!-- wp:dynamic /-->␊
␉ <!-- wp:static-child -->␊
␉ <div class="static-child">Last child</div>␊
␉ <!-- /wp:static-child -->␊
␉ </div>␊
<!-- /wp:static -->Becomes:
␊
␉ <div class="static">␊
␉ ␊
␉ <div class="static-child">First child</div>␊
␉ ␊
␉ <p class="dynamic">Hello World!</p>␊
␉ ␊
␉ <div class="static-child">Last child</div>␊
␉ ␊
␉ </div>␊I've noticed here that the indentation was inconsistent, with the static block's contents being indented, but static-child block's content remains at the same indentation:
wordpress-develop/tests/phpunit/tests/blocks/wpBlock.php
Lines 371 to 383 in 6ba48db
| $block_markup = ' | |
| <!-- wp:static --> | |
| <div class="static"> | |
| <!-- wp:static-child --> | |
| <div class="static-child">First child</div> | |
| <!-- /wp:static-child --> | |
| <!-- wp:dynamic /--> | |
| <!-- wp:static-child --> | |
| <div class="static-child">Last child</div> | |
| <!-- /wp:static-child --> | |
| </div> | |
| <!-- /wp:static --> | |
| '; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the test code itself performs an initial
trim()on the supplied test data. Could that be making these more confusing than they need to be.
The trim() doesn't seem to have any impact, perhaps because I've manually removed leading whitespace. I'll remove it.
That
@todolooks like a good candidate forWP_Block_Processor::extract_full_block_and_advance()
I agree, however this revealed an issue with that function. This is a bugfix I'd like to ensure is included in 6.9.1, so I'd prefer to avoid nonessential changes like this. I'm happy to create another PR for that.
This comment was marked as spam.
This comment was marked as spam.
ockham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for fixing this. I had one suggestion that could be applied throughout, but it's also fine if we land this as-is rather than futzing around any more with whitespace.
dmsnell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the text indentation is changes without needing to be changed, e.g. de-indenting all of the lines with the NOWDOC stings. I don’t think there’s reason to block this change, but it would be much smaller perhaps if those didn’t move. From the HTML perspective it shouldn’t matter, as those all have at least one whitespace character (the newline).
My approval is for either choice you make, to directly merge, or to update and merge.
I don't think there's a solution that produces smaller output. it's a question of picking between the version without indentation (what I've proposed) and a version where the indentation appears in the result (because it is present in the rendered block's HTML). I chose this version because the sequence of mixed newlines and tabs led to lines where the only text was a tab. Lines like that are often flagged by linters or removed by editors and are generally surprising. My solution to that (in a previous commit in this PR) was to include a tab character reference in the expected HTML (see <div class="static">
	
<div class="static-child">First child</div>
	
<p class="dynamic">Hello World!</p>
	
<div class="static-child">Last child</div>
	
</div>I preferred removing the indentation from the source rather than attempting to maintain it in the expected output. |
Ensure that leading text whitespace is included in
assertEqualHTMLcomparisons. See the Trac ticket description:Trac ticket: https://core.trac.wordpress.org/ticket/64531
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.