search

liuggio / fastest

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

Add symfony 5 support #143

Closed vincentmoulene closed 4 years ago

vincentmoulene commented 4 years ago

Add Symfony 5 support #SymfonyHackday

rbaarsma commented 4 years ago

@vincentmoulene why is there no changed files? This pr seems to add nothing?

vincentmoulene commented 4 years ago

@rbaarsma sorry for that. Now it's updated.

One of my first contribution for the open source ;-)

rbaarsma commented 4 years ago

@vincentmoulene Tests fail with Symfony 5 it seems.

DonCallisto commented 4 years ago

@vincentmoulene as @rbaarsma said, the test are failing. Please try to take a look. If you need help we can review this togheter in the next weeks.

vincentmoulene commented 4 years ago

@DonCallisto I don't know the module. So may be need some help ;-). If you can advice me, I spend a bit some time.

DonCallisto commented 4 years ago

As you can see from Travis failing build you're in the case where something has changed in Sf5 about a parameter typehint

PHP Fatal error: Uncaught TypeError: Argument 1 passed to Symfony\Component\Process\Process::__construct() must be of the type array, string given, called in /home/travis/build/liuggio/fastest/src/Process/ProcessFactory.php on line 55 and defined in /home/travis/build/liuggio/fastest/vendor/symfony/process/Process.php:140 Stack trace:

0 /home/travis/build/liuggio/fastest/src/Process/ProcessFactory.php(55): Symfony\Component\Process\Process->__construct('vendor/phpunit/...', NULL, Array)

1 /home/travis/build/liuggio/fastest/src/Process/ProcessFactory.php(33): Liuggio\Fastest\Process\ProcessFactory->createProcess('vendor/phpunit/...', Array)

2 /home/travis/build/liuggio/fastest/src/Process/ProcessesManager.php(56): Liuggio\Fastest\Process\ProcessFactory->createAProcess(Object(Liuggio\Fastest\Queue\TestSuite), 1, 1, true)

3 /home/travis/build/liuggio/fastest/src/Command/ParallelCommand.php(142): Liuggio\Fastest\Process\ProcessesManager->assertNProcessRunning(Object(Liuggio\Fastest\Queue\Infrastructure\InMemoryQueue), Object( in /home/travis/build/liuggio/fastest/vendor/symfony/process/Process.php on line 140

You should adapt the call in ProcessFactory when creating a new Process.

vincentmoulene commented 4 years ago

@DonCallisto => I will check during the week. I have some time on friday. If it's ok for you.

DonCallisto commented 4 years ago

@vincentmoulene open source is not about how quick we do the things so pay no mind, is up to you and your time, we all have a private life. Just a thing, if you won't be able to go on with this (no more time, no more commitment, no more desire, and so on and so on) just write here and let all know that this PR is "open" for other to contribute. Thanks!

DonCallisto commented 4 years ago

@vincentmoulene my bad, didn't check the implementation. I see that we can use the static function fromShellCommandline in order to keep previous behaviour (the string as a cli command) but there's two things we should take care of:

1) I don't know how and if is possible to stub static methods in PHPUnit (I usually write tests with PHPSpec) 2) Don't know how to escape properly the parameters (check the documentation about this method).

In order to keep using __construct the second point on the list seems still to be a problem. Don't know, have to think about it carefully but if you have an idea or an implementation solution you're welcome!

DonCallisto commented 4 years ago

Ok, maybe the solution is to split the command by space when those spaces are not wrapped inside ' ', " " or \ Have to check if three supported OS (linux,osx,windows) follow this convention.

webhdx commented 4 years ago

Any update on this? I can help if @vincentmoulene is not keen on finishing it. What is exactly missing here? I think solution from PR is actually good enough. It will make one element array from strings which should work with Process without any problems.

DonCallisto commented 4 years ago

@webhdx look at my last two comments

DonCallisto commented 4 years ago

See https://github.com/liuggio/fastest/pull/149

DonCallisto commented 4 years ago

@vincentmoulene @rbaarsma check #149 please and give it a try and a feedback. Thanks.