Fix test order (1) - Never write into Fixtures dir (see #2516)

Description ----------- | Q | A | -----------------| --- | Fixed issues | partly solves #2316 | Docs PR or issue | - I'm working on test isolation. This part makes sure tests aren't reading temporary data from each other. This effectively means that a test should never write into the `Fixtures` dir (or anywhere else inside the source directories). If temporary files need to be created this now will happen inside the test's own `tmp` dir. While working on these files, I'm also adjusting the following for consistency: 1) Refactoring path operations to use `Path` 1) Removing private property `$filesystem` (if we want to share the `Filesystem` instance across tests, we should imo add it to `ContaoTestCase` as a static property) 1) Use `$filesystem` instead of `$fs` everywhere Following changes needed for #2316 (but unrelated to writing files) are also part of this PR: 1) Reset template loader every time (was missing once, now in `tearDown`). 1) Do not share `ContaoCacheWarmer` instance. Commits ------- 4e5fa52 never write into tests/Fixture dir baf27c4 always reset TemplateLoader 827d5f2 add missing file c3c0e62 backport changes from #1918 2aeb2ee CS 7927286 Merge branch '4.9' into bugfix/test-order/do-not-write-in-fixture-dir a50db5c Fix the prefer-lowest tests 71e7cfa clear var directory after running functional tests c466d7e bugfix for bugfix/test-order/do-not-write-in-fixture-dir 3b4fdb4 Merge pull request #2 from fritzmg/fix-bugfix/test-order/do-not-write-in-fixture-dir bugfix for bugfix/test-order/do-not-write-in-fixture-dir 9891c55 fix filepath comparisons a25d552 Merge pull request #3 from fritzmg/fix-imagefactorytest-filepath-comparisons Fix filepath comparisons
contao · Mar 15, 2021 · 9fea640 · 9fea640
1 parent 58174af
commit 9fea640
Show file tree

Hide file tree

Showing 17 changed files with 257 additions and 249 deletions.
diff --git a/composer.json b/composer.json
@@ -118,6 +118,7 @@
         "twig/twig": "^2.7",
         "ua-parser/uap-php": "^3.9",
         "webignition/robots-txt-file": "^3.0",
+        "webmozart/path-util": "^2.2",
         "wikimedia/less.php": "^1.7"
     },
     "replace": {

diff --git a/core-bundle/composer.json b/core-bundle/composer.json
@@ -104,6 +104,7 @@
         "twig/twig": "^2.7",
         "ua-parser/uap-php": "^3.9",
         "webignition/robots-txt-file": "^3.0",
+        "webmozart/path-util": "^2.2",
         "wikimedia/less.php": "^1.7"
     },
     "conflict": {

diff --git a/core-bundle/src/Image/LegacyResizer.php b/core-bundle/src/Image/LegacyResizer.php
@@ -54,18 +54,15 @@ public function resize(ImageInterface $image, ResizeConfiguration $config, Resiz
             $this->legacyImage = null;
             $legacyPath = $image->getPath();
 
-            if (0 === strpos($legacyPath, $projectDir.'/') || 0 === strpos($legacyPath, $projectDir.'\\')) {
+            if (Path::isBasePath($projectDir, $legacyPath)) {
                 $legacyPath = substr($legacyPath, \strlen($projectDir) + 1);
                 $this->legacyImage = new LegacyImage(new File($legacyPath));
                 $this->legacyImage->setTargetWidth($config->getWidth());
                 $this->legacyImage->setTargetHeight($config->getHeight());
                 $this->legacyImage->setResizeMode($config->getMode());
                 $this->legacyImage->setZoomLevel($config->getZoomLevel());
 
-                if (
-                    ($targetPath = $options->getTargetPath())
-                    && (0 === strpos($targetPath, $projectDir.'/') || 0 === strpos($targetPath, $projectDir.'\\'))
-                ) {
+                if (($targetPath = $options->getTargetPath()) && Path::isBasePath($projectDir, $targetPath)) {
                     $this->legacyImage->setTargetPath(substr($targetPath, \strlen($projectDir) + 1));
                 }
 

diff --git a/core-bundle/src/Resources/contao/library/Contao/StringUtil.php b/core-bundle/src/Resources/contao/library/Contao/StringUtil.php
@@ -12,6 +12,7 @@
 
 use Patchwork\Utf8;
 use Psr\Log\LogLevel;
+use Webmozart\PathUtil\Path;
 
 /**
  * Provides string manipulation methods
@@ -1112,10 +1113,12 @@ public static function trimsplit($strPattern, $strString)
 	 */
 	public static function stripRootDir($path)
 	{
-		$projectDir = System::getContainer()->getParameter('kernel.project_dir');
+		// Compare normalized version of the paths
+		$projectDir = Path::normalize(System::getContainer()->getParameter('kernel.project_dir'));
+		$normalizedPath = Path::normalize($path);
 		$length = \strlen($projectDir);
 
-		if (strncmp($path, $projectDir, $length) !== 0 || \strlen($path) <= $length || ($path[$length] !== '/' && $path[$length] !== '\\'))
+		if (strncmp($normalizedPath, $projectDir, $length) !== 0 || \strlen($normalizedPath) <= $length || $normalizedPath[$length] !== '/')
 		{
 			throw new \InvalidArgumentException(sprintf('Path "%s" is not inside the Contao root dir "%s"', $path, $projectDir));
 		}

diff --git a/core-bundle/tests/Cache/ContaoCacheWarmerTest.php b/core-bundle/tests/Cache/ContaoCacheWarmerTest.php
@@ -22,27 +22,25 @@
 use Symfony\Component\Config\Exception\FileLocatorFileNotFoundException;
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\Filesystem\Filesystem;
+use Webmozart\PathUtil\Path;
 
 class ContaoCacheWarmerTest extends TestCase
 {
-    /**
-     * @var ContaoCacheWarmer
-     */
-    private $warmer;
-
-    protected function setUp(): void
+    public static function setUpBeforeClass(): void
     {
-        parent::setUp();
+        parent::setUpBeforeClass();
 
-        $this->warmer = $this->getCacheWarmer();
+        (new Filesystem())->mkdir([
+            Path::join(self::getTempDir(), 'var/cache'),
+            Path::join(self::getTempDir(), 'other'),
+        ]);
     }
 
     protected function tearDown(): void
     {
         parent::tearDown();
 
-        $fs = new Filesystem();
-        $fs->remove($this->getFixturesDir().'/var/cache/contao');
+        (new Filesystem())->remove(Path::join($this->getTempDir(), 'var/cache/contao'));
     }
 
     public function testCreatesTheCacheFolder(): void
@@ -54,65 +52,65 @@ public function testCreatesTheCacheFolder(): void
             ->willThrowException(new FileLocatorFileNotFoundException())
         ;
 
-        $container = $this->getContainerWithContaoConfiguration($this->getFixturesDir());
+        $container = $this->getContainerWithContaoConfiguration($this->getTempDir());
         $container->set('database_connection', $this->createMock(Connection::class));
         $container->set('contao.resource_locator', $resourceLocator);
 
         System::setContainer($container);
 
         $warmer = $this->getCacheWarmer();
-        $warmer->warmUp($this->getFixturesDir().'/var/cache');
-
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/config');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/config/autoload.php');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/config/config.php');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/config/templates.php');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/dca');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/dca/tl_test.php');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/languages');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/languages/en');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/languages/en/default.php');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/sql');
-        $this->assertFileExists($this->getFixturesDir().'/var/cache/contao/sql/tl_test.php');
+        $warmer->warmUp(Path::join($this->getTempDir(), 'var/cache'));
+
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/config'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/config/autoload.php'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/config/config.php'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/config/templates.php'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/dca'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/dca/tl_test.php'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/languages'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/languages/en'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/languages/en/default.php'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/sql'));
+        $this->assertFileExists(Path::join($this->getTempDir(), 'var/cache/contao/sql/tl_test.php'));
 
         $this->assertStringContainsString(
             "\$GLOBALS['TL_TEST'] = true;",
-            file_get_contents($this->getFixturesDir().'/var/cache/contao/config/config.php')
+            file_get_contents(Path::join($this->getTempDir(), 'var/cache/contao/config/config.php'))
         );
 
         $this->assertStringContainsString(
             "'dummy' => 'templates'",
-            file_get_contents($this->getFixturesDir().'/var/cache/contao/config/templates.php')
+            file_get_contents(Path::join($this->getTempDir(), 'var/cache/contao/config/templates.php'))
         );
 
         $this->assertStringContainsString(
             "\$GLOBALS['TL_DCA']['tl_test'] = [\n",
-            file_get_contents($this->getFixturesDir().'/var/cache/contao/dca/tl_test.php')
+            file_get_contents(Path::join($this->getTempDir(), 'var/cache/contao/dca/tl_test.php'))
         );
 
         $this->assertStringContainsString(
             "\$GLOBALS['TL_LANG']['MSC']['first']",
-            file_get_contents($this->getFixturesDir().'/var/cache/contao/languages/en/default.php')
+            file_get_contents(Path::join($this->getTempDir(), 'var/cache/contao/languages/en/default.php'))
         );
 
         $this->assertStringContainsString(
             "\$this->arrFields = array (\n  'id' => 'int(10) unsigned NOT NULL auto_increment',\n);",
-            file_get_contents($this->getFixturesDir().'/var/cache/contao/sql/tl_test.php')
+            file_get_contents(Path::join($this->getTempDir(), 'var/cache/contao/sql/tl_test.php'))
         );
     }
 
     public function testIsAnOptionalWarmer(): void
     {
-        $this->assertTrue($this->warmer->isOptional());
+        $this->assertTrue($this->getCacheWarmer()->isOptional());
     }
 
     public function testDoesNotCreateTheCacheFolderIfThereAreNoContaoResources(): void
     {
         $warmer = $this->getCacheWarmer(null, null, 'empty-bundle');
-        $warmer->warmUp($this->getFixturesDir().'/var/cache/contao');
+        $warmer->warmUp(Path::join($this->getTempDir(), 'other'));
 
-        $this->assertFileNotExists($this->getFixturesDir().'/var/cache/contao');
+        $this->assertFileNotExists(Path::join($this->getTempDir(), 'var/cache/contao'));
     }
 
     public function testDoesNotCreateTheCacheFolderIfTheInstallationIsIncomplete(): void
@@ -130,9 +128,9 @@ public function testDoesNotCreateTheCacheFolderIfTheInstallationIsIncomplete():
         ;
 
         $warmer = $this->getCacheWarmer($connection, $framework);
-        $warmer->warmUp($this->getFixturesDir().'/var/cache/contao');
+        $warmer->warmUp(Path::join($this->getTempDir(), 'var/cache/contao'));
 
-        $this->assertFileNotExists($this->getFixturesDir().'/var/cache/contao');
+        $this->assertFileNotExists(Path::join($this->getTempDir(), 'var/cache/contao'));
     }
 
     /**
@@ -149,7 +147,7 @@ private function getCacheWarmer(Connection $connection = null, ContaoFramework $
             $framework = $this->mockContaoFramework();
         }
 
-        $fixtures = $this->getFixturesDir().'/vendor/contao/'.$bundle.'/Resources/contao';
+        $fixtures = Path::join($this->getFixturesDir(), 'vendor/contao/'.$bundle.'/Resources/contao');
 
         $filesystem = new Filesystem();
         $finder = new ResourceFinder($fixtures);

diff --git a/core-bundle/tests/Command/MigrateCommandTest.php b/core-bundle/tests/Command/MigrateCommandTest.php
@@ -22,6 +22,7 @@
 use Symfony\Component\Config\FileLocator;
 use Symfony\Component\Console\Tester\CommandTester;
 use Symfony\Component\Filesystem\Filesystem;
+use Webmozart\PathUtil\Path;
 
 class MigrateCommandTest extends TestCase
 {
@@ -65,7 +66,7 @@ public function testExecutesPendingMigrations(): void
      */
     public function testExecutesRunOnceFiles(): void
     {
-        $runOnceFile = $this->getFixturesDir().'/runonceFile.php';
+        $runOnceFile = Path::join($this->getTempDir(), 'runonceFile.php');
 
         (new Filesystem())->dumpFile($runOnceFile, '<?php $GLOBALS["test_'.self::class.'"] = "executed";');
 
@@ -84,6 +85,7 @@ public function testExecutesRunOnceFiles(): void
         $this->assertSame(0, $code);
         $this->assertRegExp('/runonceFile.php/', $display);
         $this->assertRegExp('/All migrations completed/', $display);
+        $this->assertFileNotExists($runOnceFile, 'File should be gone once executed');
     }
 
     public function testExecutesSchemaDiff(): void
@@ -218,7 +220,7 @@ private function getCommand(array $pendingMigrations = [], array $migrationResul
         return new MigrateCommand(
             $migrations,
             $fileLocator,
-            $this->getFixturesDir(),
+            $this->getTempDir(),
             $this->createMock(ContaoFramework::class),
             $installer ?? $this->createMock(Installer::class)
         );

diff --git a/core-bundle/tests/Command/ResizeImagesCommandTest.php b/core-bundle/tests/Command/ResizeImagesCommandTest.php
@@ -23,22 +23,26 @@
 use Symfony\Bridge\PhpUnit\ClockMock;
 use Symfony\Component\Console\Tester\CommandTester;
 use Symfony\Component\Filesystem\Filesystem;
+use Webmozart\PathUtil\Path;
 
 class ResizeImagesCommandTest extends TestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        (new Filesystem())->mkdir(Path::join($this->getTempDir(), 'assets/images'));
+    }
+
     protected function tearDown(): void
     {
         parent::tearDown();
 
-        $fs = new Filesystem();
-        $fs->remove($this->getFixturesDir().'/assets/images');
+        (new Filesystem())->remove(Path::join($this->getTempDir(), 'assets/images'));
     }
 
     public function testExecutesWithoutPendingImages(): void
     {
-        $fs = new Filesystem();
-        $fs->mkdir($this->getFixturesDir().'/assets/images');
-
         $storage = $this->createMock(DeferredImageStorageInterface::class);
         $storage
             ->method('listPaths')
@@ -56,9 +60,6 @@ public function testExecutesWithoutPendingImages(): void
 
     public function testResizesImages(): void
     {
-        $fs = new Filesystem();
-        $fs->mkdir($this->getFixturesDir().'/assets/images');
-
         $factory = $this->createMock(ImageFactoryInterface::class);
         $factory
             ->method('create')
@@ -90,9 +91,6 @@ public function testResizesImages(): void
 
     public function testTimeLimit(): void
     {
-        $fs = new Filesystem();
-        $fs->mkdir($this->getFixturesDir().'/assets/images');
-
         $factory = $this->createMock(ImageFactoryInterface::class);
         $factory
             ->method('create')
@@ -143,7 +141,7 @@ private function getCommand(ImageFactoryInterface $factory = null, DeferredResiz
         return new ResizeImagesCommand(
             $factory ?? $this->createMock(ImageFactoryInterface::class),
             $resizer ?? $this->createMock(DeferredResizerInterface::class),
-            $this->getFixturesDir().'/assets/images',
+            Path::join($this->getTempDir(), 'assets/images'),
             $storage ?? $this->createMock(DeferredImageStorageInterface::class)
         );
     }