LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL

Created on 6 October 2023, 9 months ago
Updated 27 June 2024, 1 day ago

Problem/Motivation

The LogActionsTest::testRescheduleMultipleLogsRelative() tests are currently failing on PostgreSQL. They pass on MySQL/MariaDB.

There was 1 failure:

1) Drupal\Tests\log\Functional\LogActionsTest::testRescheduleMultipleLogsRelative
Logs have been rescheduled
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 1694100128
-    1 => 1694186528
-    2 => 1694272928
+    0 => '1694272928'
+    1 => '1694186528'
+    2 => '1694100128'
 )

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/modules/contrib/log/tests/src/Functional/LogActionsTest.php:298
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 6, Assertions: 119, Failures: 1.

I traced it to the fact that $logs = $this->storage->loadMultiple(); is returning logs in reverse order... but only in this one function. Everywhere else it returns them in the correct order. I don't know why that is.

Steps to reproduce

See: https://www.drupal.org/pift-ci-job/2780177

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs review

Version

2.0

Component

Code

Created by

🇺🇸United States m.stenta

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @m.stenta
  • Assigned to Shreya_98
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.4 + Environment: PHP 8.2 & MySQL 8
    last update 9 months ago
    21 pass
  • @shreya_th opened merge request.
  • Issue was unassigned.
  • Status changed to Needs review 9 months ago
  • Status changed to Needs work 9 months ago
  • 🇺🇸United States m.stenta

    Thank you for the merge request @Shreya_th! It looks like it fixes the issue and it makes sense.

    I would still like to know *why* this is necessary, though. And why *isn't* it necessary in the other tests?

    Why is there inconsistency in the order of results returned by loadMultipl() in the first place?

    My concern is that, even though this MR fixes the tests, will it start happening in the other tests in the future? I would like to understand the underlying logic before we apply any fixes, so that we can be confident that they are the right fixes long-term.

    I wonder if this is a known behavior of Drupal core. Perhaps we can find another issue or documentation that describes the same behavior.

  • First commit to issue fork.
  • Merge request !18Run tests on multiple dbs → (Open) created by paul121
  • Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8
    last update 1 day ago
    Waiting for branch to pass
  • Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8
    last update 1 day ago
    Waiting for branch to pass
  • Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8
    last update 1 day ago
    Waiting for branch to pass
  • 🇺🇸United States paul121 Spokane, WA

    I updated the gitlab CI to run on multiple DBs inlcuding pgsql-13.5 and could reproduce this issue: https://git.drupalcode.org/project/log/-/pipelines/210166

    Why is there inconsistency in the order of results returned by loadMultiple() in the first place?

    Indeed this is weird. I wonder if it has to do with the order the logs are modified in the tests and postgres uses modified time as a default sort? But loadMultiple() actually returns an associative array with log IDs as the keys, so I'm not sure the order of the key/values in the array is guaranteed.

    I've updated the tests so that the $timestamp arrays are keyed by the log IDs. I think this is a more explicit test anyways: we want to guarantee certain logs have certain timestamps, this code isn't responsible for returning logs in any particular order.

    I'd like to merge these changes so that we can have tests running on multiple DBs (without failures!) What do you think @mstenta?

  • Status changed to Needs review 1 day ago
  • 🇺🇸United States paul121 Spokane, WA
  • 🇺🇸United States paul121 Spokane, WA

    Oh, and I didn't change the other tests that use similar $timestamp arrays. In 🐛 Update owner of cloned logs to the current user Needs review I've actually refactored how these particular action tests work so they are simpler. But LogActionsTest::testRescheduleMultipleLogsRelative() is particularly complex and can still use this approach, I think :-)

Production build 0.69.0 2024