- Issue created by @m.stenta
- Assigned to Shreya_98
- last update
9 months ago 21 pass - @shreya_th opened merge request.
- Issue was unassigned.
- Status changed to Needs review
9 months ago 7:55am 9 October 2023 - Status changed to Needs work
9 months ago 1:20pm 9 October 2023 - 🇺🇸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.
- Open on Drupal.org →Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8last update
1 day ago Waiting for branch to pass - Open on Drupal.org →Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8last update
1 day ago Waiting for branch to pass - Open on Drupal.org →Core: 10.2.1 + Environment: PHP 8.2 & MySQL 8last 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 9:54pm 27 June 2024 - 🇺🇸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 :-)