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

The PestAdapter builds the PHPUnit configuration with the pest version instead of the PHPUnit version #1795

Open
MOuli90 opened this issue Jan 4, 2023 · 5 comments
Labels

Comments

@MOuli90
Copy link

MOuli90 commented Jan 4, 2023

Question Answer
Infection version 0.26.16
Test Framework version PHPUnit 9.5.27 and pestphp/pest 1.22.3
PHP version 8.1.14
Platform Doesn't matter
Github Repo -

I am running infection with the test runner pestphp/pest. While I was looking for a strange behaivor I could find a small (not intended?) bug. The PestAdapter uses the pest version instead of the PHPUnit version to create the phpunit config files.

The partial stacktrace should be a prove

#0 ...\vendor\infection\infection\src\TestFramework\AbstractTestFrameworkAdapter.php(111): Infection\TestFramework\AbstractTestFrameworkAdapter->retrieveVersion()
#1 ...\vendor\infection\infection\src\TestFramework\AbstractTestFrameworkAdapter.php(121): Infection\TestFramework\AbstractTestFrameworkAdapter->getVersion()
#2 ...\vendor\infection\infection\src\TestFramework\AbstractTestFrameworkAdapter.php(76): Infection\TestFramework\AbstractTestFrameworkAdapter->buildInitialConfigFile()
#3 ...\vendor\infection\infection\src\TestFramework\PhpUnit\Adapter\PhpUnitAdapter.php(108): Infection\TestFramework\AbstractTestFrameworkAdapter->getInitialTestRunCommandLine('--coverage-xml=...', Array, false)
#4 ...\vendor\infection\infection\src\TestFramework\PhpUnit\Adapter\PestAdapter.php(89): Infection\TestFramework\PhpUnit\Adapter\PhpUnitAdapter->getInitialTestRunCommandLine('--coverage-xml=...', Array, false)
...

This will result in calling pest --version with the output:

Pest    1.22.3
PHPUnit 9.5.27 by Sebastian Bergmann and contributors.

an results into 1.22.3 wich is totaly correct, but not usable for the MutationConfigBuilder.

TLDR: The configuration for a unit test will not include any PHPUnit Version related configuration adjustments like enabling result cache, set the execution order etc.

Let me know if i have to share more informations

@MOuli90
Copy link
Author

MOuli90 commented Jan 4, 2023

I see two posible solutions, maybe you have some more as maintainer ;)

  1. Creating a configuration builder for Pest
  • should be possible but maybe not the best solution -> pest could require diffrent PHPUnit Versions, so the "Version match logic" could be a little bit strage
  1. Use two diffrent Version "reader" one for the output of the current pest version and one for the current PHPUnit version, that should be used to build the configuration. I think this should be the prefered one

@maks-rafalko
Copy link
Member

maks-rafalko commented Jan 8, 2023

I see the bug. However, I don't think changing the very base interface (TestFrameworkAdapter) or abstract class is a good way, because Pest is the only one exception that works in this way, relying internally on other test framework adapter.

The third alternative solution I had in my mind after some thinking is to update PestFrameworkAdapter here

public function getVersion(): string
{
return $this->phpUnitAdapter->getVersion();
}

For example first time it's called - return Pest version (to be displayed on console), second time it's called - return underlying PHPUnit version, to build configuration file.

It's dirty and hacky, however requires no changes in abstract files, which are really not needed for any test frameworks except Pest.

@theofidry
Copy link
Member

theofidry commented Jan 8, 2023

@maks-rafalko why no returning a combination within getVersion(): string? Pest X; PHPUnit Y? This way we have the info we need both as a dev and a user, since the underlying PHPUnit version matters to the user too

@maks-rafalko
Copy link
Member

maks-rafalko commented Jan 9, 2023

@maks-rafalko why no returning a combination within getVersion(): string? Pest X; PHPUnit Y? This way we have the info we need both as a dev and a user, since the underlying PHPUnit version matters to the user too

Because we need to display Pest version on terminal when Infection is executed

try {
$version = $this->testFrameworkAdapter->getVersion();
} catch (InvalidArgumentException) {
$version = 'unknown';
}
$this->output->writeln([
'',
'Running initial test suite...',
'',
sprintf(
'%s version: %s',
$this->testFrameworkAdapter->getName(),
$version
),
]);

and we need to use PHPUnit version in our code, when deciding whether to use new PHPUnit features or not.

if (version_compare($version, '10', '>=')) {
$this->configManipulator->addOrUpdateCoverageIncludeNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);
return;
}
if (version_compare($version, '9.3', '<')) {
$this->configManipulator->addOrUpdateLegacyCoverageWhitelistNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);
return;
}
// For versions between 9.3 and 10.0, fallback to version provider
if (version_compare($this->versionProvider->provide($xPath), '9.3', '>=')) {
$this->configManipulator->addOrUpdateCoverageIncludeNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);
return;
}
$this->configManipulator->addOrUpdateLegacyCoverageWhitelistNodes($xPath, $this->srcDirs, $this->filteredSourceFilesToMutate);

So in different cases we need different versions (Pest, PHPUnit).

@theofidry
Copy link
Member

Because we need to display Pest version on terminal when Infection is executed

I don't feel it's a problem to show:

Pest version: 1.2.3 (PHPUnit 9.5.6)

For the config builder it's harder to test, but I imagine we could actually have a PestConfigBuilder than can parse the PHPUnit version from the string and then leverage the PHPUnit builder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants