Opened 2 years ago
Last modified 2 years ago
#56021 new enhancement
Rename or deprecate assertDiscardWhitespace()
Reported by: | SergeyBiryukov | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Build/Test Tools | Keywords: | |
Focuses: | Cc: |
Description
There are a few custom assertion methods in WP_UnitTestCase_Base
:
assertWPError()
assertNotWPError()
assertIXRError()
assertNotIXRError()
assertEqualFields()
assertDiscardWhitespace()
assertSameIgnoreEOL()
assertEqualsIgnoreEOL()
assertSameSets()
assertEqualSets()
assertSameSetsWithIndex()
assertEqualSetsWithIndex()
assertNonEmptyMultidimensionalArray()
assertQueryTrue()
assertDiscardWhitespace()
is the odd one out, as it does not match the naming of other methods. Existing since [760/tests], it appears to have never been used in the test suite.
I think we can rename it to assertEqualsDiscardWhitespace()
and keep the old name as an alias just in case, or deprecate it altogether, maybe suggesting assertSameIgnoreEOL()
as a replacement, though it's not quite the same.
While it is possible that plugins may use the method in their tests, searching in the directory shows zero results.
Change History (8)
#2
in reply to:
↑ 1
@
2 years ago
Replying to swissspidy:
GitHub Code Search returns a few results
Thanks! Looking through the first few pages, most instances appear to be (old) copies of the WordPress core test framework, but there is some legitimate usage as well. The first option (renaming and keeping the old name as an alias) should be the way to go then. 👍
#3
@
2 years ago
Note: The Writing PHP Tests page should be updated to mention the new name when this ticket is resolved.
#4
follow-up:
↓ 5
@
2 years ago
Putting this out here for consideration:
PHPUnit 10 will introduce two PHPUnit native functions along the same lines:
Added
- #4641:
assertStringEqualsStringIgnoringLineEndings()
andassertStringContainsStringIgnoringLineEndings()
Ref: https://github.com/sebastianbergmann/phpunit/blob/main/ChangeLog-10.0.md
Once PHPUnit 10 comes out, the PHPUnit Polyfills will provide polyfills for those methods.
So instead of renaming the WP native methods, we may want to consider deprecating them altogether in favour of using the PHPUnit native versions (and the polyfills of those for older PHPUnit versions).
#5
in reply to:
↑ 4
@
2 years ago
Replying to jrf:
PHPUnit 10 will introduce two PHPUnit native functions along the same lines:
Added
- #4641:
assertStringEqualsStringIgnoringLineEndings()
andassertStringContainsStringIgnoringLineEndings()
Ref: https://github.com/sebastianbergmann/phpunit/blob/main/ChangeLog-10.0.md
Thanks! This seems like it has to do more with assertSameIgnoreEOL()
and assertEqualsIgnoreEOL()
, which are not changing at this time but can indeed be replaced with these new functions when they are available.
assertDiscardWhitespace()
is a bit different and still seems worth renaming to mention what exactly is being asserted and the underlying assertEquals()
usage.
#7
@
2 years ago
Leaving this here as it seems loosely related.
I've just had a look at the assertDiscardWhitespace()
method in a little more detail (to see how it differs) and am wondering about the whitespace replacement being done for both $actual
as well as $expected
:
$expected = preg_replace( '/\s*/', '', $expected );
This replaces all whitespace with an empty string. That effectively means that two substrings with different meaning could be considered the same.
Simple example:
$this->assertDiscardWhitespace(
'the value must be set inline',
'the value must be set in line' // . $line_number
);
I wonder if it would be more accurate to replace all whitespace with a single space instead ?
The down-side of that would be that the below would no longer be considered "equal":
$this->assertDiscardWhitespace(
'<span>something</span>',
'<span>
something
</span>'
);
Maybe the function's whitespace handling shouldn't be changed, but the docblock should be updated with a warning using it with non-markup text instead ?
Plugins shouldn't ship with test files, so I'd say this is expected.
GitHub Code Search returns a few results:
https://github.com/search?q=assertDiscardWhitespace&type=code
https://cs.github.com/?q=assertDiscardWhitespace