-
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 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 --> | |
| '; |
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.