Skip to content

Use assertSame() instead of assertEquals() where possible #3

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

Closed
mhujer opened this issue Jan 26, 2018 · 3 comments
Closed

Use assertSame() instead of assertEquals() where possible #3

mhujer opened this issue Jan 26, 2018 · 3 comments

Comments

@mhujer
Copy link
Contributor

mhujer commented Jan 26, 2018

Repost of sebastianbergmann/phpunit-documentation#494.


I noticed that the introductory documentation chapter uses assertEquals(). I suggest that it can be replaced with assertSame(), which is stricter a may prevent unexpected type conversion bugs.

What do you think?

If you think it is a good idea, I can prepare a PR.

@keradus
Copy link
Contributor

keradus commented Jan 26, 2018

big 👍

@sebastianbergmann
Copy link
Owner

Go for it.

@mhujer
Copy link
Contributor Author

mhujer commented Jan 27, 2018

I started changing the documentation, but when I was testing the example output I ran into an issue with the array not being diffed when assertSame is used.

See the example from docs:

<?php
use PHPUnit\Framework\TestCase;

class ArrayDiffTest extends TestCase
{
    public function testEquality() {
        $this->assertEquals(
            [1, 2,  3, 4, 5, 6],
            [1, 2, 33, 4, 5, 6]
        );
    }
}

It outputs this, so you can easily tell what's the problem:

1) ArrayDiffTest::testEquality
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 1
     1 => 2
-    2 => 3
+    2 => 33

But when the assertEquals() is replaced with assertSame(), the output is the following, which is pretty hard to digest.

2) ArrayDiffTest::testEquality2             
Failed asserting that Array &0 (            
    0 => 1                                  
    1 => 2                                  
    2 => 33                                 
    3 => 4                                  
    4 => 5                                  
    5 => 6                                  
) is identical to Array &0 (                
    0 => 1                                  
    1 => 2                                  
    2 => 3                                  
    3 => 4                                  
    4 => 5                                  
    5 => 6                                  
). 

There is already an issue describing this sebastianbergmann/phpunit#2169 (and the older one sebastianbergmann/phpunit#1252).

I will try to have a look at the diffing issue first, because it would be hard to persuade users to use a stricter assert, if it provides worse DX.

EDIT: PR with the fix: sebastianbergmann/phpunit#2976

nicolas-grekas added a commit to symfony/symfony that referenced this issue Feb 20, 2020
…omponent tests (GromNaN)

This PR was merged into the 3.4 branch.

Discussion
----------

Use strict assertSame instead of assertEquals in Asset component tests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #35762 (comment)
| License       | MIT
| Doc PR        | N/A

Using `assertSame` instead of `assertEquals` is recommended when possible (see sebastianbergmann/phpunit-documentation-english#3). It is stricter and must be more efficient ([`===`](https://github.com/sebastianbergmann/phpunit/blob/8.5.2/src/Framework/Constraint/IsIdentical.php#L63) vs a [comparator class](https://github.com/sebastianbergmann/phpunit/blob/8.5.2/src/Framework/Constraint/IsEqual.php#L79)).

~~Also, removing useless string cast.~~

Commits
-------

e8f3e84 Use strict assertion in asset tests
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

No branches or pull requests

3 participants