Improve test DX *and* confidence: stop using VFS

Well that's wild and deeply concerning … it passes locally:

/opt/homebrew/bin/php /private/var/folders/x1/mlh998q15c70vs7_y8s1mgqm0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/core/core/phpunit.xml --filter "/(Drupal\\Tests\\package_manager\\Kernel\\FixtureManipulatorTest::testAddPackage)( .*)?$/" --test-suffix FixtureManipulatorTest.php /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel
Testing started at 1:49 PM ...
PHPUnit 8.5.31 by Sebastian Bergmann and contributors.

Testing /Users/wim.leers/core/modules/contrib/automatic_updates/package_manager/tests/src/Kernel
.                                                                   1 / 1 (100%)

Time: 1.92 seconds, Memory: 8.00 MB

OK (1 test, 10 assertions)
Process finished with exit code 0

… apparently the sort order is not guaranteed?! 🤯

Well … fortunately we can fix that. Pushed commit that should be green 🤞

I addressed your feedback on my MR.

I am not a fan at all of disabling the use of VFS while debugging because then it's no longer the same test. It's possible a problem is VFS-specific.

If we can run tests without VFS, why don't we just do that always? That would also eliminate an aspect that @phenaproxima raised at https://drupal.slack.com/archives/C7QJNEY3E/p1673357826065869?thread_ts=... to be a reason for #3332256: package_manager_bypass should *minimally* bypass php-tuf/composer-stager, not maximally:

@wimleers (he/him)
I’ll join scrum, I have insight here. I agree with the overall reasoning, though.

phenaproxima
19 minutes ago
As I recall, the reason we need(ed) to avoid calling the Composer executable is because we use vfsStream. If we call the Composer executable, it doesn’t have the virtual filesystem available, and therefore it is not running in the same context as the tests.

phenaproxima
18 minutes ago
One way around that would be to…use the real filesystem, in microcosm. Then we could probably just stop bypassing Composer Stager.

All green, ready for final review by @tedbow.

I didn't realize this was so close to working on non-VFS! Just 10 failures, nice!

Let's make the switch to non-VFS then, and also get rid of your MR's useVFS option. Per @phenaproxima that will also pave the path for getting rid of package_manager_bypass 🤓

Yay for peeling away layers of simulation!

FYI: the only mention of vfs in all of Project Browser is over at \Drupal\Tests\project_browser\Unit\ProjectBrowserFixtureHelperTest, but that's a unit test unrelated to any of this! 👍

That means this won't disrupt PB either 🤩

In Slack, @kunal.sachdev wrote:

1. Locally it’s failing with unlink(/var/folders/h1/nxmyg30915sbfc_dh87xwh440000gn/T/package_manager_testing_roottest60262574/stage/SslDURqm1xo8oiRdS1aCk5H5BfHYgYPN63KEz_IHIP8/composer.lock): Permission denied but on drupal.org it’s failing with rmdir(/tmp/package_manager_testing_roottest52583432/stage/rQnQpFQp4WfK35_YvBj6vQaYHp9qIDNYZVWO5MHVCgs/core): Permission denied

That's an interesting difference.

2. With VFS the unlink() is returning FALSE and with

if (!$this->unlink($path)) {
  $this->logger->error("Failed to unlink file '%path'.", ['%path' => $path]);
  throw new FileException("Failed to unlink file '$path'.");
}

the error is logged but with Real file system unlink() is returning a warning (with the help of PHPUNIT) and that’s why the error is not logged.

Wow!

So this really did uncover a very significant bug that was being masked by our use of VFS in tests! \org\bovigo\vfs\vfsStreamWrapper::unlink() indeed returns FALSE, but not when calling unlink() on the real filesystem.

Bumping to critical thanks to this finding!

Currently checking if this is a known problem in \Drupal\Core\File\FileSystem::unlink() by analyzing https://www.drupal.org/project/issues/drupal?component=file+system

1. Let's create a file:
   

   $ echo dfsdfsdf > /tmp/test.txt
   $ ls -al /tmp/test.txt 
   -rw-r--r--  1 wim.leers  wheel  9 Jan 16 15:02 /tmp/test.txt
   

2. Let's remove write access from it, and let's avoid that the fact that I am the owner already will allow me to do it anyway, so make some other user + group the owner:
   

   $ chmod 0500 /tmp/test.txt
   $ ls -al /tmp/test.txt 
   -r-x------  1 wim.leers  wheel  9 Jan 16 15:02 /tmp/test.txt
   $ sudo chown root:wheel /tmp/test.txt
   Password:
   $ ls -al /tmp/test.txt 
   -r-x------  1 root  wheel  9 Jan 16 15:02 /tmp/test.txt
   

3. Alright. Now let's create a test.php file:
   

   <?php
   var_dump(error_reporting());
   var_dump(E_ALL);
   //error_reporting(E_ALL & ~E_WARNING);
   try {
        $result = unlink('/tmp/test.txt');
        var_dump($result);
   }
   catch (\Throwable $t) {
       var_dump($t);
   }
   

4. Finally, let's see how this behaves:
   

   $ php test.php
   int(32767)
   int(32767)
   PHP Warning:  unlink(/tmp/test.txt): Permission denied in /private/tmp/test.php on line 5
   
   Warning: unlink(/tmp/test.txt): Permission denied in /private/tmp/test.php on line 5
   bool(false)
   

   👆This is exactly the problem we're seeing today!

5. So … what happens if we uncomment the third line, which sets the error reporting level to E_ALL ("all errors") except for E_WARNING, resulting in "all errors except warnings"? Well:
   

   $ php test.php
   int(32767)
   int(32767)
   bool(false)
   $ ls -al /tmp/test.txt 
   -r-x------  1 root  wheel  9 Jan 16 15:02 /tmp/test.txt
   

Conclusion: we should do error_reporting(E_ALL & ~E_WARNING); in the ::setUp() of this test — because we're simulating what the behavior will be like on production, where E_ALL ought to be disabled (or enabled but then always logged and continue execution as normal).

#19 shows that this should not be critical because it does not impact production sites. Still, it means our testing will be much closer to reality :)

7 remarks on the MR. Nice progress here :)

AFAICT

1) Drupal\Tests\package_manager\Kernel\StageOwnershipTest::testStageDestroyedWithFileSystemError
rmdir(/tmp/package_manager_testing_roottest90314029/stage/I4yoveM9CXTEXZuHAE82mEh4xb3sd6Av4MbKnrxlY_w/core): Permission denied

is happening because previously $stage->destroy() deleted a VFS directory, but now it is deleting one on the real filesystem. On DrupalCI, that directory is owned by a different user, unlike on your local development environment. I think.

Just pushed debug output based on https://www.php.net/manual/en/function.chown.php#example-2195.

Found yet another reason to do this:

    // Pre-load the contents of composer.json so that Factory::createComposer()
    // won't try to call realpath(), which will fail if composer.json is in a
    // virtual file system.
    $configuration = $dir . DIRECTORY_SEPARATOR . 'composer.json';
    if (file_exists($configuration)) {
      $configuration = file_get_contents($configuration);
      $configuration = json_decode($configuration, TRUE, 512, JSON_THROW_ON_ERROR);
    }

👆 This causes \Drupal\package_manager\Validator\ComposerPatchesValidator to wrongly conclude some config.json file contains the information, even though it's composer.json! 🤯

This blocks #3332256: package_manager_bypass should *minimally* bypass php-tuf/composer-stager, not maximally.

#23 can and should already be addressed here too, and should result in changes to \Drupal\Tests\package_manager\Kernel\ComposerPatchesValidatorTest.

This likely makes #3156467: [upstream] Updates fail if OS temp directory is a symlink obsolete.

I know this isn't marked as Needs review yet, but because this is the single most important issue right now, I went ahead and alredy did a review. 20 remarks on the MR, most trivial to address (and largely about simplifying the changes), and a bunch of questions.

@kunal.sachdev and I just paired to investigate whether we'd be able to remove the PathLocator::setPaths() call like I said I'd hoped to be able to do in yesterday's review of the MR 🤓

Turns out we can't, because in kernel tests, PathLocator::getVendorDirectory() will always point to the test runner's site, not the system under test's site — because otherwise you'd end up with A) a lot of file copying, B) the impossibility for the same code to be executed for both the test runner and the SUT, because they'd need different class loaders.

Conclusion: my idea was idle hope to get rid of package_manager_bypass' PathLocator override 😅 Well at least we tried!

Now doing a deep final review!

👍 0 remaining strings mentioning virtual, 4 remaining for vfs but they're all necessary.
1. 👎 A lot of the things I asked for wrt simplifying/reducing changes have not been implemented.
2. 🥳 Tests are passing!

Given point 2 impedes A) blocks many other issues, B) slows down final review + commit by @tedbow, C) reduces likelihood of conflicts, I took that on myself.

I took this from

288 insertions(+), 303 deletions(-)

to

214 insertions(+), 221 deletions(-)

Please land this before your EOD @tedbow 🙏

Thank you for the finishing touches!

This unblocked:

• #3332256: package_manager_bypass should *minimally* bypass php-tuf/composer-stager, not maximally
• #3331168: Limit trusted Composer plugins to a known list, allow user to add more (one other blocker remains) → #3319679: Assert known preconditions for test runs and fail early if unmet is also less blocked!
• #3334552: Ensure that intentionally failed test stage directories are deleted after tests complete (one other blocker remains)

And it seems to confirm #3156467-6: [upstream] Updates fail if OS temp directory is a symlink.