liuggio / fastest

Simple parallel testing execution... with some goodies for functional tests.
MIT License
475 stars 65 forks source link

Allow doctrine/collections:^2.0 as alternative dependency #187

Closed toniperic closed 1 year ago

toniperic commented 1 year ago

Given this, seems no harm to include major version 2 as an alternative dependency.

I only noticed this as one of my projects wanted to upgrade to doctrine/collections:^2.0 but couldn't because liuggio/fastest depends exclusively on major version 1.

janklan commented 1 year ago

Yes Please!

DonCallisto commented 1 year ago

It seems that build is failing https://github.com/liuggio/fastest/actions/runs/4171694587/jobs/7221915682

mnocon commented 1 year ago

We have the same failure in our CI pipeline:

PHP Fatal error:  Uncaught TypeError: Liuggio\Fastest\Queue\TestsQueue::add(): Return value must be of type bool, null returned in /var/www/vendor/liuggio/fastest/src/Queue/TestsQueue.php:56
Stack trace:
#0 /var/www/vendor/liuggio/fastest/src/Queue/CreateTestsQueueFromSTDIN.php(51): Liuggio\Fastest\Queue\TestsQueue->add()
#1 /var/www/vendor/liuggio/fastest/src/Queue/CreateTestsQueueFromSTDIN.php(32): Liuggio\Fastest\Queue\CreateTestsQueueFromSTDIN->addLineIfNotEmpty()
#2 /var/www/vendor/liuggio/fastest/src/Queue/ReadFromInputAndPushIntoTheQueue.php(28): Liuggio\Fastest\Queue\CreateTestsQueueFromSTDIN->execute()
#3 /var/www/vendor/liuggio/fastest/src/Command/ParallelCommand.php(101): Liuggio\Fastest\Queue\ReadFromInputAndPushIntoTheQueue->execute()

The v1 of Collections used to return true after every add execution: https://github.com/doctrine/collections/blob/1.8.x/lib/Doctrine/Common/Collections/ArrayCollection.php#L313-L326

But now it doesn't return anything: https://github.com/doctrine/collections/blob/2.1.x/src/ArrayCollection.php#L326-L329

I can prepare a PR to this repository, changing the return value of add to true: https://github.com/liuggio/fastest/blob/master/src/Queue/TestsQueue.php#L45-L57

to keep BC.

Is that ok or do you have another suggestion?

DonCallisto commented 1 year ago

If I'm not mistaken we never return that boolean value. It's true that a change in return type could be a BC but only if someone explicitly extend from that class.

Possible solutions:

WDYT?

mnocon commented 1 year ago

I don't think this small change justifies releasing a new major.

I would go for option 2 (trigger a deprecation warning, return true and drop the bool return value in next major), I feel it's the best of two worlds - we keep the v1 working and are ready to make changes for v2 in the future.

I can prepare a PR with the changes if you agree

DonCallisto commented 1 year ago

Yes please, go on with option 2

mnocon commented 1 year ago

I've created a PR: https://github.com/liuggio/fastest/pull/188

DonCallisto commented 1 year ago

Resolved with https://github.com/liuggio/fastest/pull/188