Intermittent test fails in LocaleUpdateTest::testUpdateImportSourceRemote()

Just combing through the majors, can this be closed now?

Actually I found #2223459: Segmentation fault, Out of memory in LocaleUpdateTest and others so tentatively closing this as a duplicate.

Reopening since this failed today: https://www.drupal.org/pift-ci-job/517457
The issue this was marked duplicate of is now marked "cannot reproduce".

The fail can also be seen here:
https://www.drupal.org/pift-ci-job/521026

1	Locale.Drupal\locale\Tests\LocaleUpdateTest
✓		- setUp
✓		- addLanguage
✓		- testUpdateProjects
✓		- testUpdateCheckStatus
✗	
testUpdateImportSourceRemote
fail: [Other] Line 138 of core/modules/locale/src/Tests/LocaleUpdateTest.php:
Updates found
✓		- testUpdateImportSourceLocal
✓		- testUpdateImportModeNonCustomized
✓		- testUpdateImportModeNone
✓		- testEnableUninstallModule
✓		- testEnableLanguage
✓		- testEnableCustomLanguage

A different random fail in the same test: https://www.drupal.org/pift-ci-job/526406

1	Locale.Drupal\locale\Tests\LocaleUpdateTest
✓		- setUp
✓		- addLanguage
✓		- testUpdateProjects
✓		- testUpdateCheckStatus
✗	
testUpdateImportSourceRemote
fail: [Other] Line 165 of core/modules/locale/src/Tests/LocaleUpdateTest.php:
Translation of February
Value 'Februar_1' is equal to value 'Februar_2'.

fail: [Other] Line 166 of core/modules/locale/src/Tests/LocaleUpdateTest.php:
Translation of March
Value 'Marz_1' is equal to value 'Marz_2'.
✓		- testUpdateImportSourceLocal
✓		- testUpdateImportModeNonCustomized
✓		- testUpdateImportModeNone
✓		- testEnableUninstallModule
✓		- testEnableLanguage
✓		- testEnableCustomLanguage

I am seeing lots of random fails in tests right now related to updates (eg, https://www.drupal.org/node/2627512#comment-11728075).

Wild thought. We have tests running in parallel. Patches with schema / container changes have empty update hooks to trigger a container rebuild. Are we getting fails because test A happens to be running while test B is doing a container rebuild in an update hook.

?

#2828045: Tests failing with unrelated „Translation: The French translation is not available” is not the same issue, but also related to l.d.o.

@catch, @alexpott, @effulgentsia, @Cottser, and I agreed that this is critical as a random test failure under normal circumstances in HEAD.

Another case https://www.drupal.org/pift-ci-job/567927, from #2828559-117: UpdatePathTestBase tests randomly failing.

So I'm pretty sure that this fail occurs when in \Drupal\locale\Tests\LocaleUpdateBase::setUp()...

    // Setup timestamps to identify old and new translation sources.
    $this->timestampOld = REQUEST_TIME - 300;
    $this->timestampMedium = REQUEST_TIME - 200;
    $this->timestampNew = REQUEST_TIME - 100;
    $this->timestampNow = REQUEST_TIME;

and the $this->timestampNow and $this->timestampNew and on different days - i.e the test runs at 00:00:01 in whatever timezone the testbot PHP is configured to run in.

in \Drupal\locale\Tests\LocaleUpdateBase::setTranslationFiles the remote file is written with a timestamp based on $this->timestampNew whereas the test is testing asserting on line 139 based on $this->timestampNow. Once the translation from the remote is downloaded the timestamp on the translation passes this assert...

$this->assertTrue($history['contrib_module_one']['de']->timestamp >= $this->timestampNow, 'Translation of contrib_module_one is imported');

so that's perhaps how this fail occurred. format_date($this->timestampNew, 'html_date') returns YEAR-MONTH-DAY so most of the time this test will pass.

Yep by playing with my system clock I managed to prove this... test on 8.2.x fails when my system clock is 14:00:00 and passes on the branch. yay.

 8.2.x  alex: ~/dev/sites/drupal8alt.dev > d8test --browser  --verbose  --class 'Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSourceRemote'

Drupal test run
---------------

Tests to be run:
  - Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSourceRemote

Test run started:
  Monday, January 16, 2017 - 14:00

Test summary
------------

Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSource  55 passes   1 fails                 15 messages

Test run duration: 9 sec

 8.2.x  alex: ~/dev/sites/drupal8alt.dev > git co  2157927
M	.htaccess
Switched to branch '2157927'
 2157927  alex: ~/dev/sites/drupal8alt.dev > d8test --browser  --verbose  --class 'Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSourceRemote'

Drupal test run
---------------

Tests to be run:
  - Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSourceRemote

Test run started:
  Monday, January 16, 2017 - 14:00

Test summary
------------

Drupal\locale\Tests\LocaleUpdateTest::testUpdateImportSource  56 passes                           15 messages

Test run duration: 9 sec

Given this bugfix, can we add some line of documentation why this value is preferred over the other?

[Possible crosspost]

I think that is an accurate assessment.

A few notes, though.

1. After lines 139-140 in testUpdateImportSourceRemote, there should be a negative assertions for the modules that shouldn't get updated. Prob OOS here?

2. Does line 156 also need to be adjusted

    $this->assertTrue($history['contrib_module_one']['de']->timestamp >= $this->timestampNow, 'Translation of contrib_module_one is imported');

Seems like it should really be

    $this->assertEqual($history['contrib_module_one']['de']->timestamp, $this->timestampNew, 'Translation of contrib_module_one is imported');

so that it explicitly make sure the local version is chosen over the remote one. In scope?

3. It looks like lines 210-215 in testUpdateImportSourceLocal should also be adjusted to be more precise, but that may be OOS?

Re #20

1. Yep out-of-scope - nothing to do with the random error
2. Nope this does not need adjusting that assert is about the timestamp of the new local file that has been downloaded from the remote. The assertion is correct.
3. Yep out-of-scope.

In my mind this patch is complete and verified fix to the random fail.

Re #19 - it is not that is is preferred it is the correct value since it is the value used by the remote source as it's timestamp. The assertion in HEAD is just wrong.

I think the assessment of what is wrong is correct, and the patch in #15 is a good fix. I am also OK with the answers in #21.

Random test fail fix subject to an unrelated random test fail :( #2844509: Random fails in BrowserTestBase tests with Operation timed out after 30001 milliseconds with 0 bytes

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