-
Notifications
You must be signed in to change notification settings - Fork 71
Improve tests, replace psr7 implementation #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
58d93aa
to
b8f83a3
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped 2.X already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
88741ab
to
61a8b2b
Compare
61a8b2b
to
060d579
Compare
|
||
namespace PHPPM\Tests\Fixtures; | ||
|
||
class ProcessSlaveDouble |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to have support for the register_file
function without having a real Slave
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be useful when creating a test that checks if a symfony container resource is actually being added to the list of watched files
@@ -9,7 +9,7 @@ | |||
use Psr\Http\Message\ServerRequestInterface; | |||
use Psr\Http\Message\ResponseInterface; | |||
use Psr\Http\Message\UploadedFileInterface; | |||
use RingCentral\Psr7; | |||
use GuzzleHttp\Psr7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's not really needed here but it reduces the number of dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to require the annotations here or aren't they implicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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
UploadFileInterface
and replace it with Guzzle (which is better maintained by the way).This is just the first step in all the planned refactors :)