Moved TEST_COMMAND to global section #203
Conversation
I did also add php 7.2
|
To see if dependencies have an incoming release with PHP 7.2, it would make sense to use PHP 7.2 here: Lines 29 to 30 in 766687e |
|
Do you plan to solve failing tests in this PR? |
fbourigault
force-pushed
the
patch-global
branch
from
fd9ddb0 to
81efd33
Compare
fbourigault
force-pushed
the
patch-global
branch
from
fcf1ad0 to
8ad73b5
Compare
|
🎉 HttplugBundle is now successfully tested against PHP 7.2
|
dbu
left a comment
dbu
left a comment
There was a problem hiding this comment.
great job! apart from the expectedException i am happy with this.
| } | ||
|
|
||
| /** | ||
| * @expectedException \Exception |
There was a problem hiding this comment.
i discussed once with sebastian bergman and he is saying he agrees that the annotation is less clean than calling setExpectedException. the reason is that if the exception is thrown too early, the test would wrongly be green. this is particularly true when we expect such a generic exception
There was a problem hiding this comment.
So, it's better to have a if/else at the right position in the test?
There was a problem hiding this comment.
why if/else? afaik the $this->setExpectedException call is the best practice way. this makes sure the exception is only considered correct when thrown after that line in the code, and you have namespace resolution.
There was a problem hiding this comment.
setExpectedException was removed in 6.0 and replaced by expectException. That's why I moved to annotation.
There was a problem hiding this comment.
Yeah, We need the annotation for BC purpose...
There was a problem hiding this comment.
We can keep this as-is and switch to expectException in next-major.
| "symfony/dom-crawler": "^2.8 || ^3.0", | ||
| "polishsymfonycommunity/symfony-mocker-container": "^1.0", | ||
| "matthiasnoback/symfony-dependency-injection-test": "^1.0", | ||
| "matthiasnoback/symfony-dependency-injection-test": "^1.0 || ^2.0", |
There was a problem hiding this comment.
do we need both versions to support the range of php versions we support?
There was a problem hiding this comment.
Yes, we can't install PHPUnit 6.0 because of a conflict on sebastian/exporter.
|
Thank you. I'm happy to merge this. |
|
What do you think @dbu? |
dbu
left a comment
dbu
left a comment
There was a problem hiding this comment.
assuming there is no forward compatibility method for expectException i'd say go for it. when such a thing is added, we can start using it. its the tests so we can change without risks
|
Thank you all! |
I did also add php 7.2