Problem/Motivation

Possible random fail in #2416763-16: Convert Url::fromUri() base:// scheme to base:

85,186 pass(es), 1 fail(s), and 1 exception(s)
Non-pass
Test name	Pass	Fail	Exception
CollapsedDrupal\migrate_drupal\Tests\d6\MigrateFileTest	7	1	1
Message	Group	Filename	Line	Function	Status
copy(/tmp/some-temp-file.jpg): failed to open stream: No such file or directorycopy('/tmp/some-temp-file.jpg', 'temporary://some-temp-file.jpg') Drupal\migrate\Plugin\migrate\destination\EntityFile->import(Object, Array) Drupal\migrate\MigrateExecutable->import() Drupal\migrate_drupal\Tests\d6\MigrateFileTest->setUp() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('589', 'Drupal\migrate_drupal\Tests\d6\MigrateFileTest')	Warning	EntityFile.php	81	Drupal\migrate\Plugin\migrate\destination\EntityFile->import()	
File /tmp/some-temp-file.jpg could not be copied to temporary://some-temp-file.jpg.	migrate	MigrateTestBase.php	141	Drupal\migrate\Tests\MigrateTestBase->display()

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#21 2417549.21.patch6.66 KBalexpott
#21 19-21-interdiff.txt570 bytesalexpott
#19 2417549.18.patch6.55 KBalexpott
#19 16-18-interdiff.txt7.1 KBalexpott
#16 2417549.16.patch6.22 KBalexpott
#16 14-16-interdiff.txt7.2 KBalexpott
#14 2417549.14.patch3.96 KBalexpott
#12 2417549-migrate-file-test-isolate-tmp.diff1.55 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Random test failure

I was able to reproduce this failure locally with the patch applied once, but not thereafter.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Active » Postponed

Postponing pending it being reproduced again.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Postponed » Active

Had it twice today when uploading patches.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Related issues: +#2417923: Migrate Tests Fail Consistently when run through UI

#2417923: Migrate Tests Fail Consistently when run through UI

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Related issues: +#2407505: [meta] Finalize the menu links (and other user-entered paths) system

I've not seen this on any patches - can close as outdated?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

@pwolanin, no, because @aspilicious confirmed seeing it.

@aspilicious, can you link the test results where you got the failure?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase for Issue: Convert Url::fromUri() base:// scheme to base: » Drupal\migrate_drupal\Tests\d6\MigrateFileTest fail in MigrateTestBase
Priority: Normal » Major
mihai7221’s picture

mihai7221 CreditAttribution: mihai7221 commented

Here is a test with this failure: https://qa.drupal.org/pifr/test/969373

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We run all migrate tests twice. if this is creating a file with a static name in the global temporary file system (the error looks like it), is it possible that we run the same test code twice at the same time, and then they get in the way of each other?

znerol’s picture

znerol CreditAttribution: znerol commented

Another one: https://qa.drupal.org/pifr/test/1020778

znerol’s picture

znerol CreditAttribution: znerol commented

I can reproduce it sometimes with HEAD and ./core/scripts/run-tests.sh --concurrency 8 [...] migrate_drupal.

Why does MigrateFileTest::setUp() alters $this->tempFilesDirectory in the first place? It is set to a per-test directory by the base test class for isolation purposes.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Active » Needs review
FileSize
2417549-migrate-file-test-isolate-tmp.diff1.55 KB
benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented

I think it needs to match up with the values in the d6 database as well. I think our long term solution to this was to move the tests into unit tests for the file destinations.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2417549.14.patch3.96 KB

A different approach - I can not get MigrateFileTest to pass locally without moving the file creation above $executable->import();. I think think the only reason HEAD is passing is that MigrateDrupal6Test is running before MigrateFileTest.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Btw the only reason #12 passed is because the testbot that ran the test already has /tmp/some-temp-file.jpg existing from a previous test run :P.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Priority: Major » Critical
FileSize
14-16-interdiff.txt7.2 KB
2417549.16.patch6.22 KB

This is a confirmed random test fail and should be critical. Discussed with @benjy in IRC and he pointed out that we should not be altering the dumps. Therefore we need to come up with a way of altering stuff once the dumps have loaded. The patch attached introduces two new interfaces to allow migrate tests to alter dumps and react to a tear down so that we don;t have to hack stuff into MigrateFullDrupalTestBase tests. The interfaces are:

  • MigrateDumpAlterInterface
  • MigrateTearDownInterface
benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented 
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateFileTest.php
@@ -90,7 +101,41 @@ public function testFiles() {
+    $random = new Random();
+    static::$tempFilename = $test_id . $random->name() . '.jpg';
...
+    Database::getConnection('default', 'migrate')->update('files')
+      ->condition('fid', 6)
+      ->fields(array(
+        'filename' => static::$tempFilename,
+        'filepath' => '/tmp/' . static::$tempFilename,
+      ))

If we put the file into $this->tempFilesDirectory rather than /tmp and updated the database accordingly could we avoid the tear down stuff altogether?

I think we'd also need to update file_temporary_path for that to work.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +D8 Accelerate Dev Days
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
16-18-interdiff.txt7.1 KB
2417549.18.patch6.55 KB

@benjy good idea.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Reviewed & tested by the community

Looks great, thanks for fixing this!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
19-21-interdiff.txt570 bytes
2417549.21.patch6.66 KB

Fixed missing comment.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Looks like a much more robust solution. Thanks a lot for tracking this down!

Looks like migrate lead signed off, so...

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

  • webchick committed 54ba27d on 8.0.x 
    Issue #2417549 by alexpott, znerol, benjy, Berdir: Drupal\migrate_drupal...

Status: Fixed » Needs work

The last submitted patch, 21: 2417549.21.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Needs work » Fixed

Yes, yes...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.