Skip to content
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

Merged
merged 43 commits into from Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
5390f63
Set XDEBUG_MODE for processes with coverage
sanmai May 4, 2021
317e5ce
Don't even mention XDEBUG_MODE
sanmai May 4, 2021
f71de68
Add a test
sanmai May 4, 2021
fd77c35
Address PCOV
sanmai May 4, 2021
90017c6
Simplify Dockerfiles
sanmai May 4, 2021
6dd84fe
Fix PHPStan
sanmai May 4, 2021
a8a3db7
Fix tests in Windows
sanmai May 4, 2021
a645bee
Reduce epsilon
sanmai May 4, 2021
99162ef
Should use XDEBUG_MODE=coverage
sanmai May 4, 2021
6f72869
Add Xdebug 2 to CI
sanmai May 4, 2021
4999b9a
Fix tests on Windows by not assuming a new line
sanmai May 4, 2021
97ba8e1
cs
sanmai May 4, 2021
5fd95df
Merge branch 'master' into pr/2020-05/autodetect-XDEBUG_MODE
sanmai Oct 15, 2021
41433ee
Revert everything to where it was
sanmai Oct 15, 2021
aa1e089
Disable XDebug on CI
sanmai Oct 15, 2021
fd56f5d
Put everything back together after we proved it works
sanmai Oct 16, 2021
8af83b2
Merge branch 'master' into pr/2020-05/autodetect-XDEBUG_MODE
sanmai Oct 16, 2021
7b12fc4
We can know that Xdebug was offloaded
sanmai Oct 16, 2021
8070082
Return useful notices
sanmai Oct 16, 2021
ceee8e2
Fix tests
sanmai Oct 16, 2021
9f6d313
Fix it
sanmai Oct 16, 2021
ae72703
Another check for Xdebug
sanmai Oct 16, 2021
c3aa985
Code Style
sanmai Oct 16, 2021
9925caf
Update OriginalPhpProcess
sanmai Oct 16, 2021
cccb00f
Fix SA issues
sanmai Oct 16, 2021
51f3d8f
Also check for PCOV and PHPDBG
sanmai Oct 16, 2021
ba71b3b
composer.lock: update deps
sanmai Oct 16, 2021
b59fe4f
Actually, undo the update
sanmai Oct 16, 2021
2d9435e
No quotes required here apparently
sanmai Oct 16, 2021
2b6b4d0
Fix e2e tests collecting coverage outside of Infection
sanmai Oct 16, 2021
a7c2f2f
Skip E2ETest if Xdebug isn't enabled
sanmai Oct 16, 2021
394e1dc
There's something wrong on Windows
sanmai Oct 16, 2021
4597545
Explain the why
sanmai Oct 16, 2021
eec4a33
Move override close to the point of use
sanmai Oct 16, 2021
e958091
Fix negation
sanmai Oct 17, 2021
db7bd12
Rename xdebug-coverage.ini
sanmai Oct 17, 2021
2b7aa7b
Merge branch 'master' into pr/2020-05/autodetect-XDEBUG_MODE
sanmai Oct 19, 2021
e363c79
Merge branch 'master' into pr/2020-05/autodetect-XDEBUG_MODE
sanmai Nov 10, 2021
50ee06f
Make it all more readable
sanmai Nov 10, 2021
17f84ee
Merge branch 'pr/2020-05/autodetect-XDEBUG_MODE' of github.com:sanmai…
sanmai Nov 10, 2021
4270a9b
Patch up to master
sanmai Nov 10, 2021
2b8a324
Fix tests
sanmai Nov 10, 2021
20a9b99
Fix this
sanmai Nov 10, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Expand Up @@ -37,7 +37,7 @@ jobs:
with:
php-version: ${{ matrix.php-version }}
coverage: ${{ matrix.coverage-driver }}
ini-values: memory_limit=512M
ini-values: memory_limit=512M, xdebug.mode=off
tools: composer:v2

- name: Get composer cache directory
Expand Down
6 changes: 6 additions & 0 deletions devTools/phpstan-src.neon
Expand Up @@ -51,6 +51,12 @@ parameters:
-
path: '../src/FileSystem/DummyFileSystem.php'
message: '#Infection\\FileSystem\\DummyFileSystem#'
-
path: '../src/Process/OriginalPhpProcess.php'
message: '#Function ini_get is unsafe to use#'
-
path: '../src/TestFramework/Coverage/CoverageChecker.php'
message: '#Function ini_get is unsafe to use#'
level: max
paths:
- ../src
Expand Down
3 changes: 3 additions & 0 deletions devTools/phpstan-tests.neon
Expand Up @@ -26,6 +26,9 @@ parameters:
message: "#^Variable method call on Infection\\\\Tests\\\\FileSystem\\\\Finder\\\\MockVendor\\.$#"
count: 1
path: ../tests/phpunit/FileSystem/Finder/TestFrameworkFinderTest.php
-
message: '#Function ini_get is unsafe to use#'
path: ../tests/phpunit/Process/OriginalPhpProcessTest.php
level: 4
paths:
- ../tests/phpunit
Expand Down
2 changes: 1 addition & 1 deletion devTools/xdebug-coverage.ini
@@ -1 +1 @@
xdebug.mode=coverage
xdebug.mode=off
sanmai marked this conversation as resolved.
Show resolved Hide resolved
21 changes: 21 additions & 0 deletions src/Process/OriginalPhpProcess.php
Expand Up @@ -35,7 +35,12 @@

namespace Infection\Process;

use function array_merge;
use Composer\XdebugHandler\PhpConfig;
use Composer\XdebugHandler\XdebugHandler;
use function extension_loaded;
use function ini_get as ini_get_unsafe;
use const PHP_SAPI;
use Symfony\Component\Process\Process;

/**
Expand All @@ -57,6 +62,22 @@ public function start(?callable $callback = null, ?array $env = null): void
$phpConfig = new PhpConfig();
$phpConfig->useOriginal();

if (
extension_loaded('pcov') ||
PHP_SAPI === 'phpdbg' ||
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
) {
// Why going through all the trouble above? We don't want to enable
// Xdebug when there are more compelling choices. In the end the user is
// still in control: they can provide XDEBUG_MODE=coverage on their own.
$env = array_merge($env ?? [], [
'XDEBUG_MODE' => 'coverage',
]);
}
Copy link
Member

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.

  1. Why is it needed to pass XDEBUG_MODE=coverage when pcov is loaded, for example?
  2. In the end the user is still in control: they can provide XDEBUG_MODE=coverage on their own. - is it true? If user passes XDEBUG_MODE=debug and Xdebug is loaded (with, let's say, off mode), then this new code will override it by XDEBUG_MODE=coverage, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This was a mistake, now corrected.
  2. 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.
  3. XDEBUG_MODE=debug is no use for us, we have to override it with XDEBUG_MODE=coverage

Copy link
Member Author

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.


parent::start($callback, $env ?? []);

$phpConfig->usePersistent();
Expand Down
2 changes: 2 additions & 0 deletions src/TestFramework/Coverage/CoverageChecker.php
Expand Up @@ -42,6 +42,7 @@
use Infection\FileSystem\Locator\FileNotFound;
use Infection\TestFramework\Coverage\JUnit\JUnitReportLocator;
use Infection\TestFramework\Coverage\XmlReport\IndexXmlCoverageLocator;
use function ini_get as ini_get_unsafe;
use const PHP_EOL;
use const PHP_SAPI;
use function Safe\preg_match;
Expand Down Expand Up @@ -175,6 +176,7 @@ private function hasCoverageGeneratorEnabled(): bool
|| XdebugHandler::isXdebugActive()
|| extension_loaded('pcov')
|| XdebugHandler::getSkippedVersion() !== ''
|| ini_get_unsafe('xdebug.mode') !== false
Copy link
Member Author

@sanmai sanmai Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we're using Xdebug 3 (as XdebugHandler won't offload Xdebug if Xdebug isn't enabled)

|| $this->isXdebugIncludedInInitialTestPhpOptions()
|| $this->isPcovIncludedInInitialTestPhpOptions();
}
Expand Down
1 change: 1 addition & 0 deletions tests/e2e/Exec_Path/run_tests.bash
Expand Up @@ -35,6 +35,7 @@ if [ "$DRIVER" = "phpdbg" ]
then
PATH=$PATH:bin phpdbg -qrr vendor/bin/phpunit --coverage-xml=coverage/coverage-xml --log-junit=coverage/junit.xml
else
export XDEBUG_MODE=coverage
PATH=$PATH:bin php vendor/bin/phpunit --coverage-xml=coverage/coverage-xml --log-junit=coverage/junit.xml
fi

Expand Down
1 change: 1 addition & 0 deletions tests/e2e/Provide_Existing_Coverage/run_tests.bash
Expand Up @@ -9,6 +9,7 @@ if [ "$DRIVER" = "phpdbg" ]
then
phpdbg -qrr $PHPUNIT
else
export XDEBUG_MODE=coverage
php $PHPUNIT
fi

Expand Down
1 change: 1 addition & 0 deletions tests/e2e/Skip_Initial_Tests/run_tests.bash
Expand Up @@ -9,6 +9,7 @@ if [ "$DRIVER" = "phpdbg" ]
then
phpdbg -qrr $PHPUNIT
else
export XDEBUG_MODE=coverage
maks-rafalko marked this conversation as resolved.
Show resolved Hide resolved
php $PHPUNIT
fi

Expand Down
2 changes: 0 additions & 2 deletions tests/phpunit/AutoReview/ProjectCode/ProjectCodeProvider.php
Expand Up @@ -65,7 +65,6 @@
use Infection\Mutant\DetectionStatus;
use Infection\Mutation\MutationAttributeKeys;
use Infection\Mutator\NodeMutationGenerator;
use Infection\Process\OriginalPhpProcess;
use Infection\Process\Runner\IndexedProcessBearer;
use Infection\TestFramework\AdapterInstaller;
use Infection\TestFramework\Coverage\JUnit\TestFileTimeData;
Expand Down Expand Up @@ -100,7 +99,6 @@ final class ProjectCodeProvider
RunCommand::class,
Application::class,
ProgressFormatter::class,
OriginalPhpProcess::class,
ComposerExecutableFinder::class,
StrykerCurlClient::class,
MutationGeneratingConsoleLoggerSubscriber::class,
Expand Down
6 changes: 5 additions & 1 deletion tests/phpunit/Console/E2ETest.php
Expand Up @@ -145,7 +145,7 @@ public function test_it_runs_on_itself(): void
}

$output = $this->runInfection(self::EXPECT_SUCCESS, [
'--test-framework-options="--exclude-group=' . self::EXCLUDED_GROUP . '"',
'--test-framework-options=--exclude-group=' . self::EXCLUDED_GROUP,
]);

$this->assertMatchesRegularExpression('/\d+ mutations were generated/', $output);
Expand Down Expand Up @@ -331,6 +331,10 @@ private function runInfection(int $expectedExitCode, array $argvExtra = []): str
$this->markTestSkipped("Infection from within PHPUnit won't run without Xdebug or PHPDBG");
}

if ('\\' === DIRECTORY_SEPARATOR) {
$this->markTestSkipped('This test can be unstable on Windows');
}

/*
* @see https://github.com/sebastianbergmann/php-code-coverage/blob/7743bbcfff2a907e9ee4a25be13d0f8ec5e73800/src/Driver/PHPDBG.php#L24
*/
Expand Down
82 changes: 82 additions & 0 deletions tests/phpunit/Process/OriginalPhpProcessTest.php
@@ -0,0 +1,82 @@
<?php
/**
* This code is licensed under the BSD 3-Clause License.
*
* Copyright (c) 2017, Maks Rafalko
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* * Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* * Neither the name of the copyright holder nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

declare(strict_types=1);

namespace Infection\Tests\Process;

use Composer\XdebugHandler\XdebugHandler;
use function extension_loaded;
use Infection\Process\OriginalPhpProcess;
use function ini_get as ini_get_unsafe;
use const PHP_SAPI;
use PHPUnit\Framework\TestCase;
use Symfony\Component\Process\Process;

final class OriginalPhpProcessTest extends TestCase
{
public function test_it_extends_symfony_process(): void
{
$process = new OriginalPhpProcess([]);

$this->assertInstanceOf(Process::class, $process);
}

public function test_it_takes_command_line(): void
{
$process = new OriginalPhpProcess(['foo']);
$this->assertStringContainsString('foo', $process->getCommandLine());
}

/**
* @group integration
*/
public function test_it_injects_xdebug_env_vars(): void
{
$process = new OriginalPhpProcess(['env']);
$process->run(null, ['TESTING' => 'test']);

if (
extension_loaded('pcov') ||
PHP_SAPI === 'phpdbg' ||
XdebugHandler::getSkippedVersion() !== '' ||
ini_get_unsafe('xdebug.mode') !== false
Copy link
Member

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 &&

Copy link
Member Author

@sanmai sanmai Oct 19, 2021

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.

Copy link
Member Author

@sanmai sanmai Nov 10, 2021

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.

) {
$this->assertStringContainsString('XDEBUG_MODE=coverage', $process->getOutput());
} else {
$this->assertStringNotContainsString('XDEBUG_MODE=coverage', $process->getOutput());
}

$this->assertStringContainsString('TESTING=test', $process->getOutput());
}
}