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
Set XDEBUG_MODE for processes with coverage #1518
Set XDEBUG_MODE for processes with coverage #1518
Conversation
src/Process/CoveredPhpProcess.php
Outdated
*/ | ||
public function start(?callable $callback = null, ?array $env = null): void | ||
{ | ||
if (!extension_loaded('pcov')) { |
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 also support phpdbg
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 can add a check for it, but they are mutually exclusive. You can't use Xdebug with PHPDBG, IIRC, so whenever we export or not this variable does not matter.
On the contrary, you can have both Xdebug and PCOV. And that's why we're making sure of not breaking stuff.
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'm voting to add add phpdbg check to this condition.
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.
Fair enough. Added.
I think I messed up something here. Let me take a day to check everything again. |
Would love to have this handled automatically, it's not a PITA to add the XDEBUG_MODE=coverage in front of the infection call, but it's a bit annoying having to add that variable for all infection when's something only needed for the intial test runs. |
And another thing we realized is that everything runs much slower if we set the XDEBUG_MODE=covefage on the infection call. Previously we used a the So this patch would probably be the best solution. |
Revert "Revert everything to where it was" This reverts commit 41433ee.
src/Process/CoveredPhpProcess.php
Outdated
XdebugHandler::getSkippedVersion() !== '' || | ||
// Any other value but false means Xdebug 3 is loaded. Xdebug 2 didn't have | ||
// it too, but it has coverage enabled at all times. | ||
ini_get_unsafe('xdebug.mode') !== false |
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.
It doesn't make sense to check for an exact value of xdebug.mode
here because it is not affected by XDEBUG_MODE
.
$env = array_merge($env ?? [], [ | ||
'XDEBUG_MODE' => 'coverage', | ||
]); | ||
} |
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.
not sure I understand this block.
- Why is it needed to pass
XDEBUG_MODE=coverage
whenpcov
is loaded, for example? In the end the user is still in control: they can provide XDEBUG_MODE=coverage on their own.
- is it true? If user passesXDEBUG_MODE=debug
and Xdebug is loaded (with, let's say,off
mode), then this new code will override it byXDEBUG_MODE=coverage
, no?
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 was a mistake, now corrected.
- I meant that if a user wants to use Xdebug to collect coverage, they can provide
XDEBUG_MODE=coverage
even if they have PCOV loaded. We can't guarantee other tools will pick this up, but at least we're not standing in the way. XDEBUG_MODE=debug
is no use for us, we have to override it withXDEBUG_MODE=coverage
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.
OK, all should be much more clear in shallExtendEnvironmentWithXdebugMode
below.
src/Process/OriginalPhpProcess.php
Outdated
// - Xdebug wasn't ever there | ||
// - or Xdebug isn't version 3+ | ||
if ( | ||
!extension_loaded('pcov') || |
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 think it should be &&
but not ||
between all the conditions.
Otherwise, if the current case is phpdbg
, the this first condition will result to true
and XDEBUG_MODE=coverage
will be added
extension_loaded('pcov') || | ||
PHP_SAPI === 'phpdbg' || | ||
XdebugHandler::getSkippedVersion() !== '' || | ||
ini_get_unsafe('xdebug.mode') !== false |
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.
as in #1518 (comment), seems like here ||
also probably needs to be replaced with &&
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.
Yeah, we need to properly DI test this, without this extension_loaded
nonsense.
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.
Three weeks later DI stuff isn't here, but it looks like I was able to figure it out: here we assume that
XDEBUG_MODE=coverage
is set unless we're running with PCOV, or under PHPDBG, or with Xdebug <3. Checking for XdebugHandler
here is a foul business: we never invoke it for tests.
…/infection into pr/2020-05/autodetect-XDEBUG_MODE
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.
🤞
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.
👏 looks more clear now
I'm getting an error since this change was merged and released:
My
The same behaviour occurs in all future releases after 0.25.4. I've extracted the phar and checked how that looks inside:
Am I doing something wrong? |
This PR:
coverage
option is set #1510XDEBUG_MODE
for processes with coverage so a user doesn't even have to know this knob existsTesting steps:
Implementation notes:
XDEBUG_MODE=coverage
it emits a warning "xdebug.mode=coverage has to be set in php.ini" and fails to generate coverage). Upgrading to 9.5.10 fixes the problem.