Random fail in CopyFileTest

Did you actually mean to write random *file* or should it be random fail? :)

Is the temp directory definitely different between test runs?

Assigning to myself for grokking and review.

Discussed with @xjm @effulgentsia @cilefen @catch, and we agreed that this is a critical issue because it's a random failure.

1. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
   @@ -180,4 +180,39 @@ protected function doImport($source_path, $destination_path, $configuration = []
   +    if (!isset($scheme)) {
   

   I'd rather this were empty() instead of isset(), to protect against anything falsy, but that's not a big deal.

2. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
   @@ -180,4 +180,39 @@ protected function doImport($source_path, $destination_path, $configuration = []
   +    if (!isset($contents)) {
   

   Ditto.

3. +++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
   @@ -180,4 +180,39 @@ protected function doImport($source_path, $destination_path, $configuration = []
   +    $this->assertTrue(is_file($filepath), t('The test file exists on the disk.'), 'Create test file');
   

   Why not use assertFileExists()?

Tagging for novices -- all I need is my nitpicks in #9 fixed.

+++ b/core/modules/migrate/tests/src/Kernel/process/CopyFileTest.php
@@ -180,4 +180,39 @@ protected function doImport($source_path, $destination_path, $configuration = []
+  function createUri($filepath = NULL, $contents = NULL, $scheme = NULL) {

Yes, FileTestBase is a bit lax in specifying the visibility of its methods - but there's no reason we can't set this protected, is there?

Otherwise, if I'm understanding this correctly (by not extending FileTestBase we get KernelTestBase's VFS root, right?) this looks good. Setting off a bunch of tests just to be sure.

8.2.x/PHP 5.5/SQLite got sibling random failure #2806697: Random fail for AlreadyInstalledException, retesting that one.

I've updated the function like protected.

Thanks!

I don't understand this patch. The exact same method exists on FileTestBase and FileTestBase is a kernel test already? Why should the issue be fixed by just copying a method? Can you explain the solution in the issue summary?

Ah, sorry.

So this fixes the problem by working around a problem with FileTestBase, but what about all the other tests using FileTestBase? How do we ensure we don't introduce another fail like this in the future when someone else extends that class?

It's not a small number; there are like 15 tests that extend FileTestBase. So if it has the potential to cause random fails because of how it interacts with the file system, I really think we should discuss it at least.

@xjm we should discuss that but probably in a new issue? We might have more test cases to convert, we might end up deprecating the base class, but this specific change seems fine and removes one of 19 critical issues.

@xjm - if catch's response makes sense to you, can you move back to RTBC?

Thanks.

I think we at least need a followup maybe?

Opened follow-up #2845122: mkdir(): File exists in BrowserHtmlDebugTrait.php:141 during BrowserTestBase. Back to RTBC now we have the follow-up opened.

Thanks for opening the follow-up.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x, thanks!

Is this happening again? I have two recent examples, but decided to post the details on the open issue #2845122-5: mkdir(): File exists in BrowserHtmlDebugTrait.php:141 during BrowserTestBase