Problem/Motivation

https://www.drupal.org/pift-ci-job/37937

testContentAdminPages
fail: [Other] Line 137 of core/modules/node/src/Tests/NodeAdminTest.php:
Value 'Basic page' is equal to value 'Article'.

fail: [Other] Line 137 of core/modules/node/src/Tests/NodeAdminTest.php:
Value 'Article' is equal to value 'Basic page'.

Fail seems to be because "last modified" times are not different among the nodes, and default sorting may differ by DB

Proposed resolution

Add a query string to force ordering by title, or set the creation times to be distinct

Remaining tasks

decide correct approach to fix

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#10 interdiff.txt848 byteseffulgentsia
#10 2573289-10.patch1.74 KBeffulgentsia
#3 2573289-3.patch1.53 KBpwolanin
#2 2573289-2.patch1.78 KBpwolanin

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 2573289-2.patch1.78 KB

Here's one possible fix.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 2573289-3.patch1.53 KB

Here's maybe an even easier fix.

I guess we were depending on a default ordering by Node ID?

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/node/src/Tests/NodeAdminTest.php
@@ -118,10 +118,13 @@ function testContentAdminSort() {
+    $time = time();

Any reason to not use REQUEST_TIME(), well I don't care at all.

dawehner’s picture

dawehner
Primary language German
commented
Component: simpletest.module » node system
Status: Needs review » Reviewed & tested by the community

It is the right thing to sort by changed explicit. It is kinda hard in how many places we have that kind of issues, sadly.

amateescu’s picture

amateescu commented

+1 for the patch in #3, I tested locally and it does fix the failure on SQLite.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
heddn’s picture

heddn
Primary language English
Location Illinois
commented 
+++ b/core/modules/node/src/Tests/NodeAdminTest.php
@@ -118,10 +118,13 @@ function testContentAdminSort() {
+    $time = time();

Question: Is time() the typical way still in D8? Or should we use REQUEST_TIME?

dawehner’s picture

dawehner
Primary language German
commented

I actually think its better to use time(), because well, its kinda an implementation detail that we have new processes for those tests. They could also all be executed after each other,
so we better use the time NOW.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new 2573289-10.patch1.74 KB
new interdiff.txt848 bytes

Patch looks good, and I'll commit it in the next hour or so if no one complains about my code comment addition here.

I was confused initially by why we were decrementing $time (since time usually moves forward from one line of code to the next). Also, when Views is disabled, the admin/content page doesn't have a sort on node.changed (or if it does, its ASC). But this test enables Views, so that's fine, I just wanted to add the comment to help others looking at this code in the future.

effulgentsia’s picture

effulgentsia commented

Adding credit to all participants, because every comment had an important consideration.

effulgentsia’s picture

effulgentsia commented
Title: Core NodeAdminTest test failing on sqlite due to unspecified node ordering » Core NodeAdminTest test failing on sqlite due to unspecified node ordering

Removing trailing space from issue title.

effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x.

  • effulgentsia committed 6e4a62c on 8.0.x 
    Issue #2573289 by pwolanin, effulgentsia, dawehner, amateescu, heddn:...

Status: Fixed » Closed (fixed)

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