LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL

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.

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?

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 :-)

Postponing this until 2.x branch tests are passing again.

Nevermind! Tests are passing. I was confused by the old DrupalCI results still showing up. :-)

@paul121 I think this all makes sense - thanks!

One thought and one request:

1. I wonder if there are any guidelines for database versions. By explicitly declaring them in .gitlab-ci.yml we are giving ourselves something else to maintain moving forward, which we didn't have before. Without this change, what are the defaults? And what would happen if this were run in a GitLab instance that's different from Drupal.org's? I assume this configuration is tightly tied to that instance?
2. The commit that fixes this issue should be called "Issue #3392223: LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL" (you can copy it from the "Credit and committing" section down below on this page). That's how we'll know how to find this discussion in the future if we need to from the commit messages.

Without this change, what are the defaults?

Is it possible to declare the database types without declaring the versions, and fall back on whatever the default versions are in the GitLab instance? I assume that's what it's doing when we don't declare anything (and falls back on a version of MySQL or MariaDB)?

I acknowledge that it is better to declare specific versions. I'm just curious how it works so we can decide what makes sense.

Without this change, what are the defaults? And what would happen if this were run in a GitLab instance that's different from Drupal.org's? I assume this configuration is tightly tied to that instance?

Yes, as I understand this template is very very tightly configured to the Drupal gitlab instance. I'm not sure what would happen if you ran this on another gitlab instance. From what I understand they have designed the template to "work out of the box" for the majority of drupal contrib modules & to always run on against latest Drupal core and default services. Depending on this template is so much less maintenance work than anything else we can do.

Though I see how database versions could create more work here but I'm not sure what we can do to avoid this. I followed the Drupal Gitlab CI template docs which links to these allowed images. In fact, I had first tried only specifying pgsql-13 and it failed because I wasn't specific enough (13.5 minor version is required). For pgsql 16 and mysql 8 it looks like they are maintaining a single major version of the image that will always use the latest minor release, but they don't have this for mariadb.

The commit that fixes this issue should be called "Issue #3392223: LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL" (you can copy it from the "Credit and committing" section down below on this page). That's how we'll know how to find this discussion in the future if we need to from the commit messages.

I just set the merge settings for this log project to use merge commit with semi linear history - is that OK? I think these descriptive commits are more valuable for describing how code is changing rather than just why. Or can we do this when we merge? Worrying about issue #s while working on PRs just makes things more confusing IMO

Merged - thanks @paul121!

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