Skip to content
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

Throw exception when ignoreCase on array #3317

Closed
wants to merge 1 commit into from

Conversation

mkasberg
Copy link
Contributor

@mkasberg mkasberg commented Oct 1, 2018

The documentation for assertContains() has 2 forms:

assertContains(mixed $needle, Iterator|array $haystack[, string $message = ''])

and

assertContains(string $needle, string $haystack[, string $message = '', boolean $ignoreCase = false])

Some users may become confused and try to pass $ignoreCase=true with
the first form. In this case, PHPUnit was silently ignoring the
parameter. Rather than silently ifnoring the parameter, we should throw
an exception to alert the user that this is an invalid parameter
combination.

There is a small concern here for a backward-compatibility break. The troublesome case is where a user was passing $ignoreCase=true and using an array argument, but their test was passing (because the case matches). However, it still seems reasonable to warn the user that a parameter is being ignored, and the fix for the user is simple - just pass false or don't pass anything.

I've added a unit test, but for clarity, here's what the output looks like:

Old Output

$ phpunit IgnoreCaseTest.php 
PHPUnit 7.3.2 by Sebastian Bergmann and contributors.

F.                                                                  2 / 2 (100%)

Time: 62 ms, Memory: 8.00MB

There was 1 failure:

1) IgnoreCaseTest::testArrayHasValueCaseInsensitive
Ignore case with array fails.
Failed asserting that an array contains 'ABCD'.

/home/mkasberg/Desktop/phpunit-test/IgnoreCaseTest.php:9

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.

New Output

$ ~/code/phpunit/phpunit IgnoreCaseTest.php 
PHPUnit 7.4-g435ec0594 by Sebastian Bergmann and contributors.

E.                                                                  2 / 2 (100%)

Time: 43 ms, Memory: 4.00MB

There was 1 error:

1) IgnoreCaseTest::testArrayHasValueCaseInsensitive
PHPUnit\Framework\Exception: ignoreCase is not allowed on Assert::assertContains when argument 2 is not a string.

/home/mkasberg/Desktop/phpunit-test/IgnoreCaseTest.php:9

ERRORS!
Tests: 2, Assertions: 1, Errors: 1.

Sample Test Case

<?php
use PHPUnit\Framework\TestCase;

class IgnoreCaseTest extends TestCase {

    public function testArrayHasValueCaseInsensitive() {
        $ignoreCase = true;
        $this->assertContains("ABCD", ["abcd"], 'Ignore case with array fails.', $ignoreCase);
    }

    public function testStringContainsCaseInsensitive() {
        $ignoreCase = true;
        $this->assertContains("ABCD", "abcd", 'Ignore case with string passes.', $ignoreCase);
    }
}

The documentation for assertContains() has 2 forms:

    assertContains(mixed $needle, Iterator|array $haystack[, string $message = ''])

and

    assertContains(string $needle, string $haystack[, string $message = '', boolean $ignoreCase = false])

Some users may become confused and try to pass `$ignoreCase=true` with
the first form. In this case, PHPUnit was silently ignoring the
parameter. Rather than silently ifnoring the parameter, we should throw
an exception to alert the user that this is an invalid parameter
combination.
@codecov-io
Copy link

codecov-io commented Oct 1, 2018

Codecov Report

Merging #3317 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3317      +/-   ##
============================================
+ Coverage     82.61%   82.62%   +<.01%     
- Complexity     3533     3534       +1     
============================================
  Files           143      143              
  Lines          9331     9333       +2     
============================================
+ Hits           7709     7711       +2     
  Misses         1622     1622
Impacted Files Coverage Δ Complexity Δ
src/Framework/Assert.php 93.84% <100%> (+0.01%) 237 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be6564e...9284964. Read the comment docs.

@mkasberg
Copy link
Contributor Author

mkasberg commented Oct 1, 2018

This is related to #3315 .

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it due to #3426.

@mkasberg mkasberg deleted the issue-3315 branch November 29, 2018 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants