Fixing ViewsDataTest::testFetchBaseTables to correctly check for sorting order [#2191533]

Updated: Comment #N

Problem/Motivation

This is part of #2165377: [meta] HHVM compatibility

fetchBaseTables is supposed to return its results ordered by weight and then title. Internally, it uses uasort, which according to PHP's documentation is not stable.

The test case in HEAD has got 4 elements with equal weight, but 2 elements with equal titles AND weight. The order for both elements are thus undefined after uasort. However, the test case assumes that the order is preserved.

Proposed resolution

This patch will change the comparison of the array keys to check for the proper order of the array instead of specifically checking the keys of the array for equality.

Remaining tasks

User interface changes

API changes

Original report by @lowjoel

fetchBaseTables is supposed to return its results ordered by weight and then title. Internally, it uses uasort, which according to PHP's documentation is not stable.

This patch will change the comparison of the array keys to check for the proper order of the array instead of specifically checking the keys of the array for equality.

Comment	File	Size	Author
#5	drupal-fix-array-order-checking-2191533-5-D8.patch	1.15 KB	Berdir
#5
#3	drupal-fix-array-order-checking-2191533-3-D8.patch	1.16 KB	lowjoel
#3
#1	drupal-fix-array-order-checking-2191533-1-D8.patch	1.15 KB	lowjoel
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

lowjoel CreditAttribution: lowjoel commented 8 February 2014 at 00:20

File	Size
drupal-fix-array-order-checking-2191533-1-D8.patch	1.15 KB

Upload patch v1

Comment #2

dawehner

German

CreditAttribution: dawehner commented 8 February 2014 at 00:25

Good catch!

+++ b/core/modules/views/tests/Drupal/views/Tests/ViewsDataTest.php
@@ -137,7 +137,12 @@ public function testFetchBaseTables() {
+    	$prev =  $base_tables[$base_tables_keys[$i - 1]];
...
+    	$this->assertTrue($prev['weight'] <= $current['weight'] && $prev['title'] <= $prev['title'], 'The tables are sorted as expected.');
...
+    	$current = $base_tables[$base_tables_keys[$i]];

we chould better use spaces instead of tabs.

Comment #3

lowjoel CreditAttribution: lowjoel commented 8 February 2014 at 00:30

File	Size
drupal-fix-array-order-checking-2191533-3-D8.patch	1.16 KB

Woops, editorfail.

Comment #4

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 8 February 2014 at 00:32

Status:	Needs review	» Needs work
Issue tags:		+VDC

Comment #5

Berdir

German

Switzerland

CreditAttribution: Berdir commented 8 February 2014 at 09:27

Issue summary:	View changes
Status:	Needs work	» Reviewed & tested by the community
Parent issue:		» #2165377: [meta] HHVM compatibility

File	Size
drupal-fix-array-order-checking-2191533-5-D8.patch	1.15 KB

Almost perfect, we use two spaces instead of 4. Here's a fix.

Confirmed this locally on HHVM, the test is passing again.

I'm apparently currently unable to compile the latest version (master) of HHVM due to the following error:

[  1%] make[2]: *** No rule to make target `hphp/third_party/folly/folly/SpookyHash.cpp', needed by `hphp/third_party/folly/CMakeFiles/folly.dir/folly/SpookyHash.cpp.o'.  Stop.
make[1]: *** [hphp/third_party/folly/CMakeFiles/folly.dir/all] Error 2

So I can't verify if the other tests (I currently have two failing tests with my version) are fixed but even if they weren't, it would be a separate issue.

Also updated the issue summary.