Skip to content

[tests] bring test suite up to PHP8 standards #1129

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 26, 2022
Merged

[tests] bring test suite up to PHP8 standards #1129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 13 additions & 27 deletions src/Test/MakerTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,12 @@

abstract class MakerTestCase extends TestCase
{
/**
* @var KernelInterface
*/
private $kernel;
private ?KernelInterface $kernel = null;

/**
* @dataProvider getTestDetails
*
* @return void
*/
public function testExecute(MakerTestDetails $makerTestDetails)
{
Expand All @@ -42,6 +41,9 @@ protected function createMakerTest(): MakerTestDetails
return new MakerTestDetails($this->getMakerInstance($this->getMakerClass()));
}

/**
* @return void
*/
protected function executeMakerCommand(MakerTestDetails $testDetails)
{
if (!class_exists(Process::class)) {
Expand Down Expand Up @@ -75,7 +77,7 @@ protected function executeMakerCommand(MakerTestDetails $testDetails)
foreach ($files as $file) {
$this->assertTrue($testEnv->fileExists($file), sprintf('The file "%s" does not exist after generation', $file));

if ('.php' === substr($file, -4)) {
if (str_ends_with($file, '.php')) {
$csProcess = $testEnv->runPhpCSFixer($file);

$this->assertTrue($csProcess->isSuccessful(), sprintf(
Expand All @@ -85,14 +87,17 @@ protected function executeMakerCommand(MakerTestDetails $testDetails)
));
}

if ('.twig' === substr($file, -5)) {
if (str_ends_with($file, '.twig')) {
$csProcess = $testEnv->runTwigCSLint($file);

$this->assertTrue($csProcess->isSuccessful(), sprintf('File "%s" has a twig-cs problem: %s', $file, $csProcess->getErrorOutput()."\n".$csProcess->getOutput()));
}
}
}

/**
* @return void
*/
protected function assertContainsCount(string $needle, string $haystack, int $count)
{
$this->assertEquals(1, substr_count($haystack, $needle), sprintf('Found more than %d occurrences of "%s" in "%s"', $count, $needle, $haystack));
Expand Down Expand Up @@ -122,8 +127,9 @@ private function hasRequiredDependencyVersions(MakerTestDetails $testDetails, Ma
return true;
}

$installedPackages = json_decode($testEnv->readFile('vendor/composer/installed.json'), true);
$installedPackages = json_decode($testEnv->readFile('vendor/composer/installed.json'), true, 512, \JSON_THROW_ON_ERROR);
$packageVersions = [];

foreach ($installedPackages['packages'] ?? $installedPackages as $installedPackage) {
$packageVersions[$installedPackage['name']] = $installedPackage['version_normalized'];
}
Expand All @@ -143,24 +149,4 @@ private function hasRequiredDependencyVersions(MakerTestDetails $testDetails, Ma

return true;
}

public static function assertStringContainsString(string $needle, string $haystack, string $message = ''): void
{
if (method_exists(TestCase::class, 'assertStringContainsString')) {
parent::assertStringContainsString($needle, $haystack, $message);
} else {
// legacy for older phpunit versions (e.g. older php version on CI)
self::assertContains($needle, $haystack, $message);
}
}

public static function assertStringNotContainsString(string $needle, string $haystack, string $message = ''): void
{
if (method_exists(TestCase::class, 'assertStringNotContainsString')) {
parent::assertStringNotContainsString($needle, $haystack, $message);
} else {
// legacy for older phpunit versions (e.g. older php version on CI)
self::assertNotContains($needle, $haystack, $message);
}
}
}
32 changes: 15 additions & 17 deletions src/Test/MakerTestDetails.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,16 @@

final class MakerTestDetails
{
private $maker;
private ?\Closure $runCallback = null;
private array $preRunCallbacks = [];
private array $extraDependencies = [];
private string $rootNamespace = 'App';
private int $requiredPhpVersion = 80000;
private array $requiredPackageVersions = [];

private $runCallback;

private $preRunCallbacks = [];

private $extraDependencies = [];

private $rootNamespace = 'App';

private $requiredPhpVersion;

private $requiredPackageVersions = [];

public function __construct(MakerInterface $maker)
{
$this->maker = $maker;
public function __construct(
private MakerInterface $maker,
) {
}

public function run(\Closure $callback): self
Expand All @@ -49,6 +42,9 @@ public function preRun(\Closure $callback): self
return $this;
}

/**
* @return string
*/
public function getRootNamespace()
{
return $this->rootNamespace;
Expand All @@ -70,6 +66,8 @@ public function addExtraDependencies(string ...$packages): self

public function setRequiredPhpVersion(int $version): self
{
@trigger_deprecation('symfony/maker-bundle', 'v1.44.0', 'setRequiredPhpVersion() is no longer used and will be removed in a future version.');
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we won't need this again? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope. But we are not using it within the code base at the moment. Because the class isn't internal - I figured we'd throw the deprecation. And if/when we need to use this (PHP 9ish) - we can either remove the deprecation or add a replacement internal method that gives us a bit more freedom.

I think we are going with a similar strategy w/ the PHPCompatUtil - we don't have a need for now (probably wont until PHP 9ish). So when we introduced the util in non-internal classes, we added a deprecation that "hey, you need to supply this util.. it'll be mandatory in the next version..." Now that we don't need it in say MakeWhatever::class, I changed the wording on the deprecation "hey, you dont need to supply this..."

That strategy kinda sucks, but I can't think of a better way w/ non internal classes. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

works for me 👍


$this->requiredPhpVersion = $version;

return $this;
Expand Down Expand Up @@ -120,7 +118,7 @@ public function getDependencyBuilder(): DependencyBuilder

public function isSupportedByCurrentPhpVersion(): bool
{
return null === $this->requiredPhpVersion || \PHP_VERSION_ID >= $this->requiredPhpVersion;
return \PHP_VERSION_ID >= $this->requiredPhpVersion;
}

public function getRequiredPackageVersions(): array
Expand Down
34 changes: 12 additions & 22 deletions src/Test/MakerTestEnvironment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,18 @@
*/
final class MakerTestEnvironment
{
private $testDetails;
private $fs;
private $rootPath;
private $cachePath;
private $flexPath;
private $path;

/**
* @var MakerTestProcess
*/
private $runnedMakerProcess;

private function __construct(MakerTestDetails $testDetails)
{
$this->testDetails = $testDetails;
private Filesystem $fs;
private bool|string $rootPath;
private string $cachePath;
private string $flexPath;
private string $path;
private MakerTestProcess $runnedMakerProcess;

private function __construct(
private MakerTestDetails $testDetails,
) {
$this->fs = new Filesystem();

$this->rootPath = realpath(__DIR__.'/../../');

$cachePath = $this->rootPath.'/tests/tmp/cache';

if (!$this->fs->exists($cachePath)) {
Expand Down Expand Up @@ -325,7 +318,7 @@ public function processReplacement(string $rootDir, string $filename, string $fi
}

$contents = file_get_contents($path);
if (false === strpos($contents, $find)) {
if (!str_contains($contents, $find)) {
if ($allowNotFound) {
return;
}
Expand Down Expand Up @@ -405,10 +398,7 @@ private function determineMissingDependencies(): array
');

$process = MakerTestProcess::create('php dep_runner.php', $this->path)->run();
$data = json_decode($process->getOutput(), true);
if (null === $data) {
throw new \Exception('Could not determine dependencies');
}
$data = json_decode($process->getOutput(), true, 512, \JSON_THROW_ON_ERROR);

unlink($this->path.'/dep_builder');
unlink($this->path.'/dep_runner.php');
Expand Down
5 changes: 4 additions & 1 deletion src/Test/MakerTestKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class MakerTestKernel extends Kernel implements CompilerPassInterface
{
use MicroKernelTrait;

private $testRootDir;
private string $testRootDir;

public function __construct(string $environment, bool $debug)
{
Expand Down Expand Up @@ -70,6 +70,9 @@ public function getRootDir(): string
return $this->testRootDir;
}

/**
* @return void
*/
public function process(ContainerBuilder $container)
{
// makes all makers public to help the tests
Expand Down
2 changes: 1 addition & 1 deletion src/Test/MakerTestProcess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
final class MakerTestProcess
{
private $process;
private Process $process;

private function __construct($commandLine, $cwd, array $envVars, $timeout)
{
Expand Down
48 changes: 18 additions & 30 deletions src/Test/MakerTestRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@

class MakerTestRunner
{
private $environment;
private $filesystem;
private $executedMakerProcess;
private Filesystem $filesystem;
private ?MakerTestProcess $executedMakerProcess = null;

public function __construct(MakerTestEnvironment $environment)
{
$this->environment = $environment;
public function __construct(
private MakerTestEnvironment $environment,
) {
$this->filesystem = new Filesystem();
}

Expand All @@ -39,6 +38,9 @@ public function runMaker(array $inputs, string $argumentsString = '', bool $allo
return $this->executedMakerProcess->getOutput();
}

/**
* @return void
*/
public function copy(string $source, string $destination)
{
$path = __DIR__.'/../../tests/fixtures/'.$source;
Expand All @@ -62,29 +64,6 @@ public function copy(string $source, string $destination)
}
}

/**
* When using an authenticator "fixtures" file, this adjusts it to support Symfony 5.2/5.3.
*/
public function adjustAuthenticatorForLegacyPassportInterface(string $filename): void
{
// no adjustment needed on 5.4 and higher
if ($this->getSymfonyVersion() >= 50400) {
return;
}

$this->replaceInFile(
$filename,
'\\Passport;',
"\\Passport;\nuse Symfony\Component\Security\Http\Authenticator\Passport\PassportInterface;"
);

$this->replaceInFile(
$filename,
': Passport',
': PassportInterface'
);
}

public function renderTemplateFile(string $source, string $destination, array $variables): void
{
$twig = new Environment(
Expand Down Expand Up @@ -116,6 +95,9 @@ public function getExecutedMakerProcess(): MakerTestProcess
return $this->executedMakerProcess;
}

/**
* @return void
*/
public function modifyYamlFile(string $filename, \Closure $callback)
{
$path = $this->getPath($filename);
Expand All @@ -130,6 +112,9 @@ public function modifyYamlFile(string $filename, \Closure $callback)
file_put_contents($path, $manipulator->getContents());
}

/**
* @return void
*/
public function runConsole(string $command, array $inputs, string $arguments = '')
{
$process = $this->environment->createInteractiveCommandProcess(
Expand Down Expand Up @@ -197,7 +182,7 @@ public function configureDatabase(bool $createSchema = true): void
// this looks silly, but it's the only way to drop the database *for sure*,
// as doctrine:database:drop will error if there is no database
// also, skip for SQLITE, as it does not support --if-not-exists
if (0 !== strpos(getenv('TEST_DATABASE_DSN'), 'sqlite://')) {
if (!str_starts_with(getenv('TEST_DATABASE_DSN'), 'sqlite://')) {
$this->runConsole('doctrine:database:create', [], '--env=test --if-not-exists');
}
$this->runConsole('doctrine:database:drop', [], '--env=test --force');
Expand Down Expand Up @@ -234,6 +219,9 @@ public function writeFile(string $filename, string $contents): void
file_put_contents($this->getPath($filename), $contents);
}

/**
* @return void
*/
public function addToAutoloader(string $namespace, string $path)
{
$this->replaceInFile(
Expand Down