antonioribeiro / google2fa

A One Time Password Authentication package, compatible with Google Authenticator.
MIT License
1.83k stars 199 forks source link

Bug in testing due to PHPUnit's interpretation #168

Closed alexw23 closed 2 years ago

alexw23 commented 3 years ago

As was building some of my own tests I noticed that where you assert verifyKeyNewer should be returning a timestamp, it actually returns a boolean.

Take the following as an example of this (Source):

$this->assertEquals(
    26213400,
    $this->google2fa->verifyKeyNewer(
        Constants::SECRET,
        '758576',
        null,
        2,
        26213400
    )
); // 26213400

It turns out that if the "timestamp" is empty verifyKeyNewer returns true not a timestamp.

And PHPunit for some reason considers these both to be equal, for some reason I have no idea.

The following example also works:

$this->assertEquals(
    26213400,
    true
); // 26213400

Just thought I would report this in an effort to have any expectation of how it should be working clarified.

Edit: looks like you need to be using assertSame.

thinkverse commented 2 years ago

And PHPunit for some reason considers these both to be equal, for some reason I have no idea.

I know it's been a long time since this issue was open. But just to clarify that PHPUnit's interpretation is not wrong here.

It's interpreting the assertion correctly, 26213400 is a truthy value in PHP. Therefore assetEquals will return true since a truthy value is equal to true in PHP. Using assertSame will fail however since the expectation and the actual value aren't of the same type, they are both truthy but one is an int and the other is a boolean.

Boiled down, PHPUnit's assertEquals is a non-strict equality check in PHP (==), assertSame would be a strict equality check (===).

This can be seen if you try and write it out the checks yourself in plain PHP, as demonstrated here.

var_dump(26213400 == true);  // true
var_dump(26213400 === true); // false