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

QA: use the most specific PHPUnit assertion possible #414

Merged
merged 1 commit into from Nov 5, 2020

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 24, 2020

This is a long established best practice.

PHPUnit contains a variety of assertions and the ones available has been extended hugely over the years.

To have the most reliable tests, the most specific assertion should be used.

Refs:

Most notably, this changes nearly all calls to assertEquals() to assertSame(), where assertEquals() does a loose type comparison and assertSame() does a strict type comparison.

Refs:


Note: this change now causes two tests to fail on PHP 5.4-5.6 and three tests to fail on PHP 5.2-5.3.

Based on these test results, the methods under test are not PHP cross-version compatible as they return different results depending on the PHP version.

We'll need to decide what the desired behaviour should be and will need to adjust the code base to fix this. Input for this very welcome.


Test failure details

The first two fail on all PHP 5.x builds. The third only on PHP 5.2 and 5.3.

There were 3 failures:
1) RequestsTest_Transport_cURL::testHEAD
Failed asserting that false is identical to ''.
/home/travis/build/rmccue/Requests/tests/Transport/Base.php:154
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
2) RequestsTest_Transport_fsockopen::testHEAD
Failed asserting that false is identical to ''.
/home/travis/build/rmccue/Requests/tests/Transport/Base.php:154
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46
3) RequestsTest_Session::testURLResolution
Failed asserting that Requests_IRI Object (
    'scheme' => 'http'
    'iuserinfo' => null
    'ihost' => 'localhost'
    'port' => 8080
    'ipath' => '/get'
    'iquery' => null
    'ifragment' => null
    'normalization' => Array (
        'acap' => Array (
            'port' => 674
        )
        'dict' => Array (
            'port' => 2628
        )
        'file' => Array (
            'ihost' => 'localhost'
        )
        'http' => Array (
            'port' => 80
        )
        'https' => Array (
            'port' => 443
        )
    )
) is identical to 'http://localhost:8080/get'.
/home/travis/build/rmccue/Requests/tests/Session.php:10
/home/travis/.phpenv/versions/5.2.17/bin/phpunit:46

@jrfnl jrfnl added this to the 1.7.1 milestone Oct 24, 2020
@schlessera
Copy link
Member

schlessera commented Nov 3, 2020

  • For the test failures RequestsTest_Transport_cURL::testHEAD and RequestsTest_Transport_fsockopen::testHEAD:

    The Requests_Response object states that the body property is of type string, so I'd consider the fact that it returns false on PHP 7.0+ for the tested request a bug. Therefore, I'd suggest we detect where the false is happening (probably some PHP internal method producing an error, maybe json_decode or so?), and then catch that case an explicitly turn it into an empty string.

  • For the failure on RequestsTest_Session::testURLResolution:

    Again, going by the fact that Requests_Response states that the url property is of type string, I'd consider this a bug. The url in this case is a Requests_IRI object and the loose comparison would trigger its __toString() method. I suggest manually calling the object's __toString() method (or just casting it to (string)) when assigning it to the url property.

This is a long established best practice.

PHPUnit contains a variety of assertions and the ones available has been extended hugely over the years.

To have the most reliable tests, the most specific assertion should be used.

Refs:
* https://phpunit.de/manual/4.8/en/appendixes.assertions.html
* https://phpunit.readthedocs.io/en/7.5/assertions.html#

Most notably, this changes nearly all calls to `assertEquals()` to `assertSame()`, where `assertEquals()` does a loose type comparison and `assertSame()` does a strict type comparison.

Refs:
* https://phpunit.de/manual/6.5/en/appendixes.assertions.html#appendixes.assertions.assertEquals
* https://phpunit.de/manual/6.5/en/appendixes.assertions.html#appendixes.assertions.assertSame
@jrfnl jrfnl force-pushed the feature/qa-tests-use-strict-assertions branch from 289fddb to 95e96ae Compare November 5, 2020 03:07
@jrfnl
Copy link
Member Author

jrfnl commented Nov 5, 2020

@schlessera I'll get those fixed up and send in separate PRs for those as those will constitute functional changes which need a changelog entry.

As for this PR, I've removed the three failing changes, so this should be mergable once the build has passed.

@schlessera schlessera merged commit cff2fcf into master Nov 5, 2020
@schlessera schlessera deleted the feature/qa-tests-use-strict-assertions branch November 5, 2020 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants