-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[PhpUnitBridge] Added support for PHPUnit 7 in Coverage Listener #26089
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
Q | A |
---|---|
Branch? | 3.4 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | |
License | MIT |
Doc PR |
if (\PHP_VERSION_ID >= 70000) { | ||
exec('type phpdbg', $output, $returnCode); | ||
|
||
if (\PHP_VERSION_ID >= 70000 && 0 === $returnCode) { |
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.
ping @nicolas-grekas : I did not have phpdbg on my machine, so I added that (IIRC, you added this part)
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.
would it work on windows?
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.
We don't care as this test is not run on windows ;)
if (class_exists('PHPUnit_Runner_Version') && version_compare(\PHPUnit_Runner_Version::id(), '6.0.0', '<')) { | ||
class_alias('Symfony\Bridge\PhpUnit\Legacy\CoverageListener', 'Symfony\Bridge\PhpUnit\CoverageListener'); | ||
} elseif (version_compare(\PHPUnit\Runner\Version::id(), '7.0.0', '>=')) { | ||
require_once __DIR__.'/CoverageListenerWithReturnTypes.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.
I don't get why I have to do that :/
Without this line, I got
PHP Warning: Class 'Symfony\Bridge\PhpUnit\CoverageListenerWithReturnTypes' not found in /home/gregoire/dev/github.com/lyrixx/symfony/src/Symfony/Bridge/PhpUnit/CoverageListener.php on line 25
How to reproduce:
- install phpunit 7+
- get my branch
cd src/Symfony/Bridge/PhpUnit/
composer install
phpunit
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.
Strange. Related: in #26139 I'm proposing to completely split implems in separate classes, and do only aliases here. I'd suggest to do the same here.
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.
okay, I will mimic your PR
bd117f7
to
8a824e6
Compare
I updated the PR to mimic existing code. |
8a824e6
to
6c0e6af
Compare
Ok, now the listener does not need anymore the The PR is now ready |
Thank you @lyrixx. |
…stener (lyrixx) This PR was merged into the 3.4 branch. Discussion ---------- [PhpUnitBridge] Added support for PHPUnit 7 in Coverage Listener | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT 8000 | Doc PR | Commits ------- 6c0e6af [PhpUnitBridge] Added support for PHPUnit 7 in Coverage Listener
|
||
public function startTestSuite(TestSuite $suite): void | ||
{ | ||
$this->trait->startTest($test); |
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 wrong, the suite variable should be used