Improve tests, replace psr7 implementation#152
Conversation
acasademont
force-pushed
the
improve_tests
branch
from
58d93aa to
b8f83a3
Compare
acasademont
force-pushed
the
improve_tests
branch
from
b8f83a3 to
88741ab
Compare
| "symfony/http-foundation": "^2.6|^3.0|^4", | ||
| "symfony/http-kernel": "^2.6|^3.0|^4", | ||
| "ringcentral/psr7": "^1.2" | ||
| "symfony/http-foundation": "^3.4|^4", |
There was a problem hiding this comment.
Dropped 2.X already
There was a problem hiding this comment.
What do you think- can we release this in a point release? Effectively we're dropping support for Symfony before 3.4?
There was a problem hiding this comment.
The proposed 'kernel.reset' interface for Symfony is only in 3.4+. Also, SF 2.x is EOL already, I'd say it's time to move on.
|
Wow, this is huge! Will need some time to check, sorry. |
|
No worries, happy to help! The test assertions haven't changed, seems big but I don't think is thaaat big :p |
| @@ -1,87 +0,0 @@ | |||
| <?php | |||
There was a problem hiding this comment.
@andig basically I copied all the code in the previous Kernel mock and put it inside the newly created controllers, that allows us to keep the assertions the same
acasademont
force-pushed
the
improve_tests
branch
from
88741ab to
61a8b2b
Compare
acasademont
force-pushed
the
improve_tests
branch
from
61a8b2b to
060d579
Compare
|
|
||
| namespace PHPPM\Tests\Fixtures; | ||
|
|
||
| class ProcessSlaveDouble |
There was a problem hiding this comment.
This is used to have support for the register_file function without having a real Slave
There was a problem hiding this comment.
will be useful when creating a test that checks if a symfony container resource is actually being added to the list of watched files
| use Psr\Http\Message\ResponseInterface; | ||
| use Psr\Http\Message\UploadedFileInterface; | ||
| use RingCentral\Psr7; | ||
| use GuzzleHttp\Psr7; |
There was a problem hiding this comment.
I guess that's not really needed here but it reduces the number of dependencies?
There was a problem hiding this comment.
This is just a change to a more complete PSR7 implementation and to a better maintained library, it doesn't really change the number of dependencies itself.
Also, the Psr7 Response object is used here. Are you refering to something else?
| "symfony/http-foundation": "^2.6|^3.0|^4", | ||
| "symfony/http-kernel": "^2.6|^3.0|^4", | ||
| "ringcentral/psr7": "^1.2" | ||
| "symfony/http-foundation": "^3.4|^4", |
There was a problem hiding this comment.
What do you think- can we release this in a point release? Effectively we're dropping support for Symfony before 3.4?
| "phpunit/phpunit": "^5.7" | ||
| "phpunit/phpunit": "^5.7", | ||
| "symfony/framework-bundle": "^3.4|^4", | ||
| "doctrine/annotations": "^1.6" |
There was a problem hiding this comment.
Do we need to require the annotations here or aren't they implicit?
There was a problem hiding this comment.
Nopes, they aren't implicit, Symfony relies on them for routing annotation but you need to import the library yourself
|
As for symfony bundle: is it a good idea to require them in the composer json? That way we will only ever be able to install a single framework instead of testing them separately. Wondering if it would make more sense to move the symfony framework to travis.yml to prepare a separate environment per framework. Apart from that LGTM. |
It's a good question! The three frameworks can live together without a problem, we're already doing that for phpstan in travis (we install Laravel, Symfony and Drupal). I'm not a Laravel or Drupal expert but I don't see why testing them should interfere with Symfony. Also, I'm not a fan of moving too much logic into Travis, imo tests should be runnable with a simple "phpunit" command. |
|
Then I would propose to merge as-is and then proceed with #144 |
2 participants
Trying to add tests for #144 proved to be quite complicated if we don't have a full Symfony kernel for the tests. Therefore, I've refactored all the current tests to stop using mocks and use a real Symfony kernel/controllers...
As a side effect, the current PSR-7 library did not support the
UploadFileInterfaceand replace it with Guzzle (which is better maintained by the way).This is just the first step in all the planned refactors :)