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

assertArraySubset with $strict=true doesn't display differences properly #3234

Closed
crhg opened this issue Aug 2, 2018 · 9 comments · Fixed by elecena/nano#2
Closed

assertArraySubset with $strict=true doesn't display differences properly #3234

crhg opened this issue Aug 2, 2018 · 9 comments · Fixed by elecena/nano#2

Comments

@crhg
Copy link

crhg commented Aug 2, 2018

Q A
PHPUnit version 7.2.7
PHP version 7.2.5
Installation Method Composer

The following test will fail but no differences will be displayed.

<?php
use PHPUnit\Framework\TestCase;

class FooTest extends TestCase
{
    public function testFoo()
    {
        $this->assertArraySubset(['foo' => 1], ['foo' => '1'], true);
    }
}
% ./vendor/bin/phpunit FooTest.php
PHPUnit 7.2.7 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 28 ms, Memory: 4.00MB

There was 1 failure:

1) FooTest::testFoo
Failed asserting that an array has the subset Array &0 (
    'foo' => 1
).
--- Expected
+++ Actual

/Users/matsui/src.nobk/phpunit-test/FooTest.php:8

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.
@lsfiege
Copy link

lsfiege commented Aug 3, 2018

+1 with PHPUnit 7.3.0

@sebastianbergmann
Copy link
Owner

I am not sure whether displaying a diff is useful here to begin with. Is

Failed asserting that an array has the subset Array &0 (
    'foo' => 1
).

not sufficient? If it is, then we should simply remove the diff completely.

@crhg
Copy link
Author

crhg commented Aug 4, 2018

If the information is only it, I can not know what kind of problem was with the actual value.

Displaying the following information at the same time makes debugging more efficient.

  • Which key has a problem
  • Whether element of that key exists or not
  • If an element of that key exists, what was the actual value?

@1blankz7
Copy link

1blankz7 commented Sep 8, 2018

The input for the diff is currently generated with r_print. The result for ['foo' => '1'] is ([foo] => 1). If we use var_export the correct type would be more present in the output and the diff is more usable.

@crhg
Copy link
Author

crhg commented Sep 9, 2018

This fix is ​​valid for this issue, but I think there is a problem when $strict is false.

When $strict is false, the number 1 and the string '1' are treated as equal, so the print_r used in the original code is better.

It is good to use print_r and var_export differently depending on whether $strict is true or false.

@1blankz7
Copy link

1blankz7 commented Sep 9, 2018

Can you explain why using var_export is an issue when $strict is false? In my understanding the code is not executed at all if the check passes.

@crhg
Copy link
Author

crhg commented Sep 9, 2018

When expected is

[
    'foo' => 1,
    'bar' => 2,
]

and actual is

[
    'foo' => '1',
    'bar' => '3',
]

the check doesn't pass and the diff is shown.

If var_export is used, both foo and bar is different, but $strict is false, foo is ok and only bar should be included in diff.

@stale
Copy link

stale bot commented Nov 8, 2018

This issue has been automatically marked as stale because it has not had activity within the last 60 days. It will be closed after 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 8, 2018
@stale
Copy link

stale bot commented Nov 15, 2018

This issue has been automatically closed because it has not had activity since it was marked as stale. Thank you for your contributions.

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 a pull request may close this issue.

4 participants