search

brick / math

Arbitrary-precision arithmetic library for PHP
MIT License
1.83k stars 78 forks source link

Fixed missing test cases and PHP version #37

Closed GrahamCampbell closed 4 years ago

GrahamCampbell commented 4 years ago

It looks like the tests were incorrectly casting some cases to a string which means the tests were not actually doing what they were meant to. I've fixed this also. NB This was highlighted because PHPUnit calls tests with strict types enabled from v8 upwards.

It's good practice to bounded version constraints (such as on the PHP versions). I've also tweaked the minimum dependency versions so that they work properly on PHP 7.4.

GrahamCampbell commented 4 years ago

Hmmm. Travis didn't run?

GrahamCampbell commented 4 years ago

https://travis-ci.org/github/brick/math/builds/670982687

BenMorel commented 4 years ago

Hi, thank you for your PR!

It looks like the tests were incorrectly casting some cases to a string which means the tests were not actually doing what they were meant to. I've fixed this also. NB This was highlighted because PHPUnit calls tests with strict types enabled from v8 upwards.

Excellent point, I thought strict_types would protect from this, but because the call is made from PHPUnit and not from the test class, it's indeed not taken into account here.

It's good practice to bounded version constraints (such as on the PHP versions). I've also tweaked the minimum dependency versions so that they work properly on PHP 7.4.

Makes sense for PHP itself, but how exactly did you pick the minimum versions for the dev dependencies? If I install with --prefer-lowest, phpunit for example works fine @ 7.0.0!

GrahamCampbell commented 4 years ago

But in the case that the tests fail, you might see phpunit crash instead. 7.5.15 was the first version to actually support PHP 7.4, and genuinely had fixes in.

BenMorel commented 4 years ago

Alright, thank you! 👍