Handle strings containing empty strings assertions

[GaryJones]

PHPUnit 6.4.2 added a fix to gracefully handle the case when an empty string was passed to assertStringContainsString() and variant assertions. Versions earlier than that gave a "mb_strpos(): Empty delimiter" error instead.

This change adds a fix to also handle that case, when using PHPUnit-Polyfills with versions of PHPUnit earlier than 6.4.2, and passing an empty needle string.

The use of asserting true is true stops the test from being marked as risky, which is what would happen if we just return true;. This is hacky, and there may be a better way for this.

Fixes #17.

[jrfnl]

The use of asserting true is true stops the test from being marked as risky, which is what would happen if we just return true;. This is hacky, and there may be a better way for this.

Would using markTestSkipped() work for this ? With a message along the lines of 'An empty string always "exists" in any other string' ?

That would:

1. Allow the test to pass without being marked as risky, nor would it throw the PHP error.
2. Still give people an indication that they are using the wrong assertion for this particular test (if they are interested enough to investigate).

[jrfnl]

@GaryJones Just thinking through some alternatives.

Another option would be to still call the underlying assertion, but for an empty $needle, pass the $haystack as both the $needle and the $haystack to the underlying function.

We may also want to consider doing a PHPUnit version check, so as to only apply the work-around when it is actually needed.

[GaryJones]

I started with the $needle = $haystack approach, but that didn't make sense for the NotContains assertions, since it would always fail (Failed asserting that 'foobar' does not contain "foobar".).

I then changed to use the "mark the test as skipped" approach, and then further built out the conditions under which this should happen into a new method.

Below PHPUnit 6.4.2, the fix is always needed, so all four empty string tests added in this PR are skipped:

Above 6.4.2, the fix is not needed for positive assertions, but is needed for the NotContains assertions, since otherwise you get Failed asserting that 'foobar' does not contain "", as an empty string will never NOT be found in another string - means we can let PHPUnit handle the positive cases, and we just skip two tests:

All was going well, and then I realised that ALL versions of PHPUnit (including above 7.5.0) were failing on the negative assertions.

So maybe I should remove the fixes for the negative assertions, and let PHPUnit 6.4.2–<7.5.0 fail like default PHPUnit does? Or maybe just remove the testAssertStringNotContainsEmptyString() and testAssertStringNotContainsEmptyStringIgnoringCase() tests completely?

[jrfnl]

Hiya @GaryJones! Nice work so far ;-)

When I look at things as they are now, there are two points I'd like to make:

1. This library is meant to polyfill and should not change the behaviour of assertions. So, yes, working around this particular PHP error is fine, but if a test would "normally" fail in PHPUnit, it should also fail when the polyfills are used.
   This ties in with your findings and the question you pose above.
2. Regarding the tests: for plain "fall through" polyfills, just testing for the availability of the function is enough. However, as these assertions would now no longer be plain fall through polyfills, the tests need to test the actual behaviour of the polyfill.
   In practice, this means that the tests will need to test that a PHPUnit\Framework\SkippedTestError/PHPUnit_Framework_SkippedTestError exception is throw for those PHPUnit versions where the fix kicks in, while testing that the assertion behaviour is the same for those PHPUnit versions without the fix.

[jrfnl]

If this method remains, it can be simplified a bit.

Two options:

1. If you'd only be looking for "is the version above 6.x.x.", you can use something along the lines of the below:

   if ( \class_exists( '\PHPUnit\Runner\Version' ) === true && \version_compare( Version::id(), '6.4.2', '>=' ) ) {}

   ... as if the \PHPUnit\Runner\Version class doesn't exist, we already know it will be PHPUnit < 6.0.0.
   This is similar to patterns as used in various places in the phpunitpolyfills-autoload.php file.
2. Alternatively, you could add a class alias for the PHPUnit_Runner_Version class to the new name in the Autoload::loadAssertStringContains() method.
   This is already done for various other PHPUnit classes in the phpunitpolyfills-autoload.php file for those polyfills which needed it.

[jrfnl]

Oh and another suggestion - if the markTestSkipped() remains, the code in the assertions can be simplified by moving the call to markTestSkipped() to this method and just doing a plain, non-conditional call to it from the assertion methods.
The method name should in that case probably be changed to something like skipOnEmptyNeedle().

[jrfnl]

Edit: Updated code snippet (due to some upcoming changes):

if ( \class_exists( '\PHPUnit_Runner_Version' ) === true || \version_compare( Version::id(), '6.4.2', '<' ) ) {}

[GaryJones]

@jrfnl Your feedback all makes sense - thank you.

I don't know when I'm going to get to this, so please feel free to take this over!

[jrfnl]

please feel free to take this over!

Thanks @GaryJones ! Don't mind if I do...

So, I've done some fiddling with this PR and have added the following commit to finish things off. Let me know if this feels right to you as well.

AssertStringContains: finish off the fix for mb_strpos() warning

As per #18 (comment). it is okay for the polyfills to work around this particular bug, but the work-around should not change the behaviour of the assertions.

With that in mind, I've made the following changes:

AssertStringContains trait

• Removed the test skipping, in favour of emulating the PHPUnit >= 6.4.2 behaviour.
  A test may be doing multiple assertions. If this particular assertion would cause the test to be skipped, the other assertions (if after this assertion) would not be run anymore. This would be a change in behaviour, which is undesirable.
• As the behaviour of the underlying assertion has not changed significantly between PHPUnit 6.4.2 < 7.5.0, the version check is not really needed and removing it simplifies the fix and makes it more performant while not impacting the functionality.
• Moved the documentation for the fix to the class docblock as the needsEmptyStringFix() method has been removed.

Notes:

• There is no return after the calls to fail() as when the assertion fails the test, it throws an exception which will automatically stop the execution of the function.
• Also note the use of static:: rather than self:: to call PHPUnit native functionality. This allows for existing method overloads in a child class of the PHPUnit native TestCase to be respected.

Tests

• Expanded the documentation for the tests so it will always be clear that these tests are for a specific fix in the polyfills.
• Added a dataprovider for two of the tests to allow for testing with different haystacks, making sure a certain edge case doesn't give any issues.
• Updated the tests for the assertStringNotContainsString() polyfills to expect a PHPUnit\Framework\AssertionFailedError exception.

[GaryJones]

I've reviewed your commit, and all makes sense! Thank you!