Rename or deprecate assertDiscardWhitespace()

[SergeyBiryukov]

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.

[swissspidy]

While it is possible that plugins may use the method in their tests, searching in the directory shows zero results.

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

[SergeyBiryukov]

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. 👍

[SergeyBiryukov]

Note: The ​Writing PHP Tests page should be updated to mention the new name when this ticket is resolved.

[jrf]

Putting this out here for consideration:

PHPUnit 10 will introduce two PHPUnit native functions along the same lines:

Added

• #4641: assertStringEqualsStringIgnoringLineEndings() and assertStringContainsStringIgnoringLineEndings()

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).

[SergeyBiryukov]

Replying to jrf:

PHPUnit 10 will introduce two PHPUnit native functions along the same lines:

Added

• ​#4641: assertStringEqualsStringIgnoringLineEndings() and assertStringContainsStringIgnoringLineEndings()

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.

[jrf]

@SergeyBiryukov 👍🏻

[jrf]

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 about using it with non-markup text instead ?