@mneudert opened this Pull Request on September 20th 2017 Member

Pull Request to integrate/validate changes made in https://github.com/piwik/travis-scripts/pull/36.

The submodule change to the fork should obviously be ignored. The "bump submodules" commit integrates changes from the travis repository after running ./console generate:travis-yml --core and should be replaced before merging with a proper submodule update.

@mattab commented on September 21st 2017 Owner

Hi @mneudert

Excellent progress :+1:

I checked all the UI tests and most can be safely overriden, except for one issue.

Most UI tests failures are due to:

  • small changes in the fonts (especially for text in italic and unicode text),
  • upgraded Mysql version (the Mysql version is displayed in the 'System summary' widget in the dashboard)
  • the web server now runs on port 3000 (seen here)
  • missing GD /freetype

Blocking issue: missing GD freetype

The missing GD/freetype issue (visible in some of the ui tests such as this System check test) is the only blocking issue because we are testing that our static graphs generate correctly in the following tests:

-> is it possible to install the GD/freetype in Travis so these screenshot tests will execute as expected?

@mattab commented on September 21st 2017 Owner

@mneudert tried to trigger a few builds but it failed due to wrong submodule. Don't really understand why your last commit triggered build OK while mine didn't :confused: Looking forward to merging this hopefully :+1:

@mneudert commented on September 21st 2017 Member

Hmpf, yeah. While testing some submodules (LoginLdap war completely unimpressed about the changes!) I found a whitespace error in a script and have missed to update the reference here. The submodule commit is gone due to a force push, nothing you did wrong ^^ I have cleaned that mess up and you should now properly be able to trigger builds.

For the GD changes I am searching for a solution. It would be quite unexpected to not be able to add the missing parts. Otherwise I would expect more users to mention the graphs are broken/not visible because of the same setup problems.

@mattab commented on September 21st 2017 Owner

@mneudert maybe we could contact Travis CI and ask them for help? They usually reply quickly and are helpful so we shouldn't hesitate in case they maybe know a solution or a tip.

@mneudert commented on September 25th 2017 Member

If we switch the UITests to php-5.6 the screenshots in question seem to generate just fine and gd_info() announces the existence of freetype.

So we could upgrade the PHP version or ask travis to change the installed PHP version to include freetype support. Or perhaps both :)

Added a php-version-flip-commit to see how a full test run holds up to my observations...

@mneudert commented on September 25th 2017 Member

Not sure anymore whether lowering any of the test requirements to php-5.5 is a good idea as there seem to be some requirements hidden in the tests:

2) Piwik\Tests\System\EcommerceOrderWithItemsTest::testImagesIncludedInTests
(This should not occur on Travis CI server as we expect these tests to run there).
Scheduled reports generated during integration tests will not contain the image graphs.
For tests to generate images, use a machine with the following specifications :
    OS = linux, PHP = 5.6 and GD = 2.1.0
@mattab commented on September 25th 2017 Owner

@mneudert fyi you can try change the requirements of the tests, so they run on Travis on 5.5. The idea behind the message is to make sure they at least run there. The reason they are restricted to one PHP version and GD is because GD has some minor pixel differences depending on the php/gd versions and we need to always run them on the same one (but we can change the versions and updated the images)

@mattab commented on September 25th 2017 Owner

Also there are a few randomly failing tests:

4 failures here


There were 4 failures:

1) Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest::testApi with data set <a href='/1'>#1</a> (array('API.getSuggestedValuesForSegment'), array(1, '2017-08-21 00:00:00', array('year'), array('dimension3', 1), '_actionScope'))
Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest: Differences with expected in '/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/processed/test__actionScope__API.getSuggestedValuesForSegment.xml'
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0"?>
 <result>
   <row>en</row>
-  <row>value3</row>
   <row>value5 3</row>
   <row/>
+  <row>value3</row>
 </result>

/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestRequest/Response.php:76
/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/SystemTestCase.php:375
/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/SystemTestCase.php:515
/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/AutoSuggestTest.php:33

2) Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest::test_getMostUsedActionDimensionValues_shouldReturnMostUsedValues
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'en'
-    1 => 'value3'
-    2 => 'value5 3'
-    3 => ''
+    1 => 'value5 3'
+    2 => ''
+    3 => 'value3'
 )

/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/AutoSuggestTest.php:71

3) Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest::test_getMostUsedActionDimensionValues_shouldApplyLimit
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'en'
-    1 => 'value3'
+    1 => 'value5 3'
 )

/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/AutoSuggestTest.php:78

4) Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest::test_getMostUsedActionDimensionValues_shouldApplyIndex
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'en_US'
-    1 => 'value5'
-    2 => '343'
-    3 => 'value5 5'
+    1 => '343'
+    2 => 'value5 5'
+    3 => 'value5'
 )

/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/AutoSuggestTest.php:92

1 failure here


There was 1 failure:

1) Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest::testApi with data set <a href='/1'>#1</a> (array('API.getSuggestedValuesForSegment'), array(1, '2017-08-21 00:00:00', array('year'), array('dimension3', 1), '_actionScope'))
Piwik\Plugins\CustomDimensions\tests\System\AutoSuggestTest: Differences with expected in '/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/processed/test__actionScope__API.getSuggestedValuesForSegment.xml'
Failed asserting that two DOM documents are equal.
--- Expected
+++ Actual
@@ @@
 <?xml version="1.0"?>
 <result>
   <row>en</row>
-  <row>value3</row>
   <row>value5 3</row>
   <row/>
+  <row>value3</row>
 </result>

/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestRequest/Response.php:76
/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/SystemTestCase.php:375
/home/travis/build/piwik/piwik/tests/PHPUnit/Framework/TestCase/SystemTestCase.php:515
/home/travis/build/piwik/piwik/plugins/CustomDimensions/tests/System/AutoSuggestTest.php:33

Solution to randomly failing tests

These issues are caused by a difference in the sorting of elements. I think caused by this PHP sort behavior documented here

If two members compare as equal, their relative order in the sorted array is undefined.

This could maybe be fixed by adding another sort where when elements have the same 'count', we could sort them in alphabetical order to ensure the order is constant and does not depend on the sort implementation?

@mattab commented on September 25th 2017 Owner

So we could upgrade the PHP version

@mneudert sounds good to upgrade the PHP version for UI tests :+1:

@mneudert commented on September 27th 2017 Member

I tried modifying the SystemTests to run with PHP 5.5 in a separate branch (build here) but I think the GD library broke it. As it seems there is just not FreeType Support for PHP 5.5 on Travis at the moment, only for PHP 5.6 or the old precise images.

Already reached out to Travis to see if that is intentional or could be resolved: https://github.com/travis-ci/travis-ci/issues/8510


From what I see in your builds all flapping tests are includede in the CustomDimensions plugin, right? That would make it easy to get them fixed as we would be looking at a rather confined space...

@mattab commented on September 27th 2017 Owner

Yes they're all tests in the customDimensions plugin.

@mneudert commented on October 5th 2017 Member

As it seems the missing FreeType support on PHP 5.5 for the new Travis infrastructure is a given. Not by intention but perhaps also not changed quickly.

For now I switched the UITests to the old infrastructure in order to have a working PHP 5.5 build in the matrix because I really don't like dropping the low end requirement. Using the custom matrix we only have to change 2 lines to change the infrastructure.

@mattab commented on October 5th 2017 Owner

FYI: There's also this failing test in the last build:

There was 1 failure:

1) Piwik\Plugins\SegmentEditor\tests\Integration\SegmentEditorTest::test_UpdateSegment

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'idsegment' => 2
+    'idsegment' => '2'
     'name' => 'NEW name'
     'definition' => 'searches==0'
     'login' => 'superUserLogin'
-    'enable_all_users' => 0
-    'enable_only_idsite' => 0
-    'auto_archive' => 0
+    'enable_all_users' => '0'
+    'enable_only_idsite' => '0'
+    'auto_archive' => '0'
     'ts_created' => '2017-10-05 18:50:'
-    'ts_last_edit' => '2017-10-05 18:51:'
-    'deleted' => 0
+    'ts_last_edit' => '2017-10-05 18:50:'
+    'deleted' => '0'
 )

@mneudert looking forward to merging this one (which requires https://github.com/piwik/plugin-CustomDimensions/issues/71 )

@mneudert commented on October 6th 2017 Member

Race conditions :heart:

As far as I remember that error did not occur in any of the previous runs. As the ts_last_edit is auto generated it might have been a simple race condition with the test taking long enough to move over to the next second.

As the update is done after generating the expected result values I can only think of modifying the result to expect not a specific time but a range check for something like "ts_created + 2 seconds". Will try something like that :+1:

@mneudert commented on October 6th 2017 Member

After reading properly I saw we are talking about minutes not seconds. Are there enough tests with such a time handling that we might benefit more from a general logic?

Thinking about "assertTimeWithinSeconds($expected, $result, $seconds)" and removing it from the other result assertions...

@mattab commented on October 8th 2017 Owner

As far as I remember that error did not occur in any of the previous runs.

nice catch. I'd say we can easily ignore it as I don't remember seeing this race condition many times...

@mattab commented on October 10th 2017 Owner

Hi @mneudert would you mind updating the submodule? the build may be green this time and we can merge this useful PR :+1:

@mneudert commented on October 11th 2017 Member

Tests look rather good to me now :+1:

When merging one should take care to remove the first two commits beforehand. After merging the travis-scripts PR a run of generate:travis-yml should properly update the file to the new state.

@mattab commented on October 11th 2017 Owner

When merging one should take care to remove the first two commits beforehand.

@mneudert I'm not sure about removing commits from PR. Would you mind merging the PR yourself maybe? I've reviewed it and happy with it :+1:

@mneudert commented on October 12th 2017 Member

Hmpf, hit the wrong button on a different page. So tests need a second run to show their results here...

(previous: https://travis-ci.org/piwik/piwik/builds/287273686)

@mneudert commented on October 12th 2017 Member

Failed screenshot tests look like some usual noise.

UIIntegrationTest_segmented_visitorlog.png failed to upload due to some "Invalid authentication key" but otherwise no diffs generated.

@sgiehl commented on October 13th 2017 Member

I gonna merge this now :tada:

This Pull Request was closed on October 13th 2017
Powered by GitHub Issue Mirror