tests failing with phpunit - fatal error for syntax issues?

[tenzap]

Hello,

I know one should use ./phpunit (hence simple-phpunit) for the tests, but actually I don't have the possibility to use it, so I run the tests by executing phpunit. I can't use simple-phpunit because that script it is not packaged in Debian and it is likely it won't be. I need to run the test because I'm trying to package your lib into an official Debian package, and we are strongly advised to run upstream's test suite in the package.

Running phpunit returns this error

PHP Fatal error:  Declaration of libphonenumber\Tests\Issues\CodeCoverageTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in php-giggsey-libphonenumber/tests/Issues/CodeCoverageTest.php on line 16
PHP Stack trace:
PHP   1. {main}() /usr/bin/phpunit:0
PHP   2. PHPUnit\TextUI\Command::main($exit = *uninitialized*) /usr/bin/phpunit:42
PHP   3. PHPUnit\TextUI\Command->run($argv = [0 => '/usr/bin/phpunit'], $exit = TRUE) /usr/share/php/PHPUnit/TextUI/Command.php:95
PHP   4. PHPUnit\TextUI\Command->handleArguments($argv = [0 => '/usr/bin/phpunit']) /usr/share/php/PHPUnit/TextUI/Command.php:110
PHP   5. PHPUnit\TextUI\TestSuiteMapper->map($configuration = class PHPUnit\TextUI\XmlConfiguration\TestSuiteCollection { private $testSuites = [0 => class PHPUnit\TextUI\XmlConfiguration\TestSuite { ... }] }, $filter = '') /usr/share/php/PHPUnit/TextUI/Command.php:389
PHP   6. PHPUnit\Framework\TestSuite->addTestFiles($fileNames = [0 => 'php-giggsey-libphonenumber/tests/Issues/CodeCoverageTest.php', 1 => 'php-giggsey-libphonenumber/tests/Issues/Issue106Test.php', 2 => 'php-giggsey-libphonenumber/tests/Issues/Issue135Test.php', 3 => 'php-giggsey-libphonenumber/tests/Issues/Issue14Test.php', 4 => 'php-giggsey-libphonenumber/tests/Issues/Issue152Test.php', 5 => 'php-giggsey-libphonenumber/tests/Issues/Issue159Test.php', 6 => 'php-giggsey-libphonenumber/tests/Issues/Issue175Test.php', 7 => 'php-giggsey-libphonenumber/tests/Issues/Issue17Test.php', 8 => 'php-giggsey-libphonenumber/tests/Issues/Issue21Test.php', 9 => 'php-giggsey-libphonenumber/tests/Issues/Issue23Test.php', 10 => 'php-giggsey-libphonenumber/tests/Issues/Issue34Test.php', 11 => 'php-giggsey-libphonenumber/tests/Issues/Issue35Test.php', 12 => 'php-giggsey-libphonenumber/tests/Issues/Issue360Test.php', 13 => 'php-giggsey-libphonenumber/tests/Issues/Issue36Test.php', 14 => 'php-giggsey-libphonenumber/tests/Issues/Issue3Test.php', 15 => 'php-giggsey-libphonenumber/tests/Issues/Issue44Test.php', 16 => 'php-giggsey-libphonenumber/tests/Issues/Issue4Test.php', 17 => 'php-giggsey-libphonenumber/tests/Issues/Issue64Test.php', 18 => 'php-giggsey-libphonenumber/tests/Issues/Issue68Test.php', 19 => 'php-giggsey-libphonenumber/tests/Issues/Issue76Test.php', 20 => 'php-giggsey-libphonenumber/tests/Issues/LocaleTest.php', 21 => 'php-giggsey-libphonenumber/tests/Issues/PHP7Test.php', 22 => 'php-giggsey-libphonenumber/tests/Issues/UKNumbersTest.php', 23 => 'php-giggsey-libphonenumber/tests/buildtools/BuildMetadataFromXmlTest.php', 24 => 'php-giggsey-libphonenumber/tests/buildtools/GeneratePhonePrefixDataTest.php', 25 => 'php-giggsey-libphonenumber/tests/buildtools/MetadataFilterTest.php', 26 => 'php-giggsey-libphonenumber/tests/carrier/PhoneNumberToCarrierMapperTest.php', 27 => 'php-giggsey-libphonenumber/tests/core/AsYouTypeFormatterTest.php', 28 => 'php-giggsey-libphonenumber/tests/core/ExampleNumbersTest.php', 29 => 'php-giggsey-libphonenumber/tests/core/MatcherTest.php', 30 => 'php-giggsey-libphonenumber/tests/core/MultiFileMetadataSourceImplTest.php', 31 => 'php-giggsey-libphonenumber/tests/core/PhoneMetadataTest.php', 32 => 'php-giggsey-libphonenumber/tests/core/PhoneNumberMatchTest.php', 33 => 'php-giggsey-libphonenumber/tests/core/PhoneNumberMatcherTest.php', 34 => 'php-giggsey-libphonenumber/tests/core/PhoneNumberTest.php', 35 => 'php-giggsey-libphonenumber/tests/core/PhoneNumberUtilTest.php', 36 => 'php-giggsey-libphonenumber/tests/core/ShortNumberInfoTest.php', 37 => 'php-giggsey-libphonenumber/tests/geocoding/PhoneNumberOfflineGeocoderTest.php', 38 => 'php-giggsey-libphonenumber/tests/prefixmapper/PrefixFileReaderTest.php', 39 => 'php-giggsey-libphonenumber/tests/timezone/PrefixTimeZonesMapTest.php', 40 => 'php-giggsey-libphonenumber/tests/timezone/UKTest.php']) /usr/share/php/PHPUnit/TextUI/TestSuiteMapper.php:67
PHP   7. PHPUnit\Framework\TestSuite->addTestFile($filename = 'php-giggsey-libphonenumber/tests/Issues/CodeCoverageTest.php') /usr/share/php/PHPUnit/Framework/TestSuite.php:530
PHP   8. PHPUnit\Util\FileLoader::checkAndLoad($filename = 'php-giggsey-libphonenumber/tests/Issues/CodeCoverageTest.php') /usr/share/php/PHPUnit/Framework/TestSuite.php:402
PHP   9. PHPUnit\Util\FileLoader::load($filename = 'php-giggsey-libphonenumber/tests/Issues/CodeCoverageTest.php') /usr/share/php/PHPUnit/Util/FileLoader.php:49
PHP  10. include_once() /usr/share/php/PHPUnit/Util/FileLoader.php:65

Would you fix your tests so that they also work with phpunit?

phpunit --version
PHPUnit 9.5.2 by Sebastian Bergmann and contributors.

[giggsey]

I can't change the tests to run natively in PHPUnit 9 as it'll break older PHP versions that rely on older PHPUnit versions. That's the point of simple-phpunit.

I'd suggest using an older version of PHPUnit

[tenzap]

What about this?

https://symfony.com/doc/current/components/phpunit_bridge.html#removing-the-void-return-type

[giggsey]

That's what simple-phpunit is doing to the phpunit version it's including.

Return types were only introduced in PHP 7, and this project still supports 5.3+.

[tenzap]

From what I understood of the URL above, it permits to add the missing :void to every call that needs it, and simple-phpunit would automatically remove it for versions that don't support it like php5.3. If this is correct, it would fix my issue and permit to run phpunit with an recent version, and simply-phpunit would make it work on older versions.

But maybe I'm wrong.

Unfortunately, on the debian build systems, it is not possible to select a specific phpunit version (other than the ones shipped in the distrib)

[giggsey]

I believe it's the other way around. It removes the void return type from the PHPUnit code itself.

[tenzap]

Do you by chance know of a workaround so that I could still run the tests with phpunit? I tried the workaround at the bottom of this § https://symfony.com/doc/current/components/phpunit_bridge.html#usage but without success (I still get the same error) :

If you don't want to use the simple-phpunit script, register the following PHPUnit event listener in your PHPUnit configuration file to get the same report about deprecations (which is created by a PHP error handler called DeprecationErrorHandler):

<!-- phpunit.xml.dist -->
<!-- ... -->
<listeners>
    <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener"/>
</listeners>

In debian the phpunit bridge for symfony is available. It is just the the simple-phpunit script that isn't

[giggsey]

The listener there is only for deprecation notices, not any help here.

The simple-phpunit script if part of the phpunit-bridge, so you should have it somewhere with that package: https://github.com/symfony/phpunit-bridge/tree/5.4/bin

[tenzap]

Ok for the listener.

Concerning the script, Debian doesn't ship it in its package and won't ship it. It is not possible to get it another way (eg from the internet) on the build systems because they don't have internet access. Moreover the script requires internet access to download the composer dependencies. So I'm stuck. Probably I'll have to patch your tests manually then for debian only.

[giggsey]

Could the packager script use regex/sed to add the return type for the tests? I assume the actual package won't include the test files.

[tenzap]

I can use sed to create a patch but in the end it has to be in the form of a patch.

Indeed the tests are unlikely to be shipped in the resultant binary package. They are useful to test the package is functional.

[giggsey]

A patch would work fine. I'd be happy to include it within the project too so it's easier to keep it up to date. I don't imagine those files will change until I drop PHP 5 support anyway.

[tenzap]

What would you do with it if won't run on php5?
It would be great if the patch was merged upstream, but I fear it can't for backwards compatibility.

[giggsey]

Would your packaged version run on PHP 5? If so, how are you running phpunit?

I can just put the patch in as tests/phpunit8-compatibility.patch (or similar)

[tenzap]

Packages are uploaded to "unstable" (which I believe has php8.1) where I guess they are built, then after some time they enter "testing", which at some time becomes "stable". Packages can be backported to current Debian stable which runs php7.4 IIRC. So it is unlikely the package will be build on a build system running php5.

phpunit is called by the build system during the "test" step/target of the script that builds the binary package from source package. So it uses the version of the build system, which I believe runs "unstable" distribution of Debian (except for backports).

As for the patch, the Debian build scripts (roughly) applies them, builds the package and then unapplies them. So in think the Debian package will anyway have to maintain its own copy of the patch to be able to apply/unapply automatically.

You may find the patch here:
https://salsa.debian.org/php-team/pear/php-giggsey-libphonenumber/-/blob/debian/latest/debian/patches/03-tests_add_void_return_type.patch

[giggsey]

Okay, I think your approach of including that patch in the debian repo (especially as you have others) makes sense.

[tenzap]

It would be much preferable if the logic could be changed upstream so that no patches are needed in Debian package, but well, I understand it can not always be done.

[giggsey]

NullPointerException is being fixed now (#515). I also don't think you need https://salsa.debian.org/php-team/pear/php-giggsey-libphonenumber/-/blob/debian/latest/debian/patches/02-debian-test.patch if you are using normal phpunit again

[tenzap]

Yes, you're right. Thanks!
That patch was a test, but then I noticed that script doesn't exist in Debian at all.

[giggsey]

Happy for this to be closed now?

[tenzap]

The package could not be submitted for approval yet, so we can't know what 'debian ftp masters' would think about the patch. Nevertheless #513 (comment) is still relevant.