pkp / pkp-github-actions

PKP Github Actions
0 stars 2 forks source link

phpunit version check error returns two failed tests #11

Open withanage opened 4 months ago

withanage commented 4 months ago

Bug

phpunit version creates 2 errors:


1) PKPComponentRouterTest::testGetRequestedContextPathWithInvalidLevel
Undefined array key 1

/home/runner/ojs/lib/pkp/classes/core/PKPRouter.inc.php:218
/home/runner/ojs/lib/pkp/tests/classes/core/PKPRouterTestCase.inc.php:93

2) PKPPageRouterTest::testGetRequestedContextPathWithInvalidLevel
Undefined array key 1

/home/runner/ojs/lib/pkp/classes/core/PKPRouter.inc.php:218
/home/runner/ojs/lib/pkp/tests/classes/core/PKPRouterTestCase.inc.php:93

It seems that the PHPUnit version check doesn't match the PHP version. In OJS 3.3, we're using PHPUnit 9.5, which is causing the tests to fail. A more consistent approach would be to verify the PHPUnit version.

Vendor Information

https://github.com/sebastianbergmann/phpunit/issues/5062

withanage commented 4 months ago

Hi @asmecher ,

I have in OJS 3.3 github actions the issue, that the above tests fai for php 8.0+, cause it assumes that the php version shoud be larger,

https://github.com/pkp/pkp-lib/pull/9788/files

But this seems to me like, it is related to the phpunit 10 support, which drops expectError()

However after my change of assuming a larger phpunit version, travis tests for ojs 3.3. , 8.0+ fails, what I think is a valiadtion error. https://app.travis-ci.com/github/pkp/ojs/builds/269371762

I tested my change with GA https://github.com/withanage/ojs/actions/runs/8189570913 and it seems to work.

I am a little unsure there, could you take a look ?

asmecher commented 4 months ago

I might be missing the point -- I haven't gone into this deeply -- but wouldn't it make sense to check both the PHP version and phpunit version? Obviously if PHPUnit doesn't have expectError it shouldn't be used -- but also I think this is a case where something that was previously a warning got promoted to an error with a new version of PHP. So perhaps both should be checked.

withanage commented 4 months ago

Yes, it colud be the proecess of Thanks Alec, restricting both would be better and stop travis tests from braking too.