Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queues - Ensure that queue timings work, even with bad tzdata #25363

Merged
merged 5 commits into from
Jan 17, 2023

Conversation

totten
Copy link
Member

@totten totten commented Jan 17, 2023

Overview

The queue tests have been reporting different results on different hosts -- e.g. they pass on test-{1,2,3} and on my laptop, but they fail on the temporary worker nodes. The difference is that tzdata does not appear to be loaded on the temp node's copy of mysqld.

This updates the queuing queries so that the correctness of tzdata does not affect the outcome.

Before

We can see that tests/phpunit/api/v4/Entity/QueueTest.php has several failures in this environment.

Note:

After

After: It doesn't matter if mysqld has tzdata. The test passes.

Comments

I'm not sure if this reflects a substantive bug for typical runtimes. It might... or it might be that the test-suite requires more unusual situations.

@civibot
Copy link

civibot bot commented Jan 17, 2023

(Standard links)

@civibot civibot bot added the master label Jan 17, 2023
@seamuslee001
Copy link
Contributor

Test fail seems to relate

TypeError: Civi\Cv\Util\ConsoleQueueRunner::formatTaskCallback(): Argument #1 ($task) must be of type CRM_Queue_Task, bool given, called in phar:///home/jenkins/bknix-dfl/bin/cv/vendor/composer/../../src/Util/ConsoleQueueRunner.php on line 76 in Civi\Cv\Util\ConsoleQueueRunner::formatTaskCallback() (line 109 of phar:///home/jenkins/bknix-dfl/bin/cv/vendor/composer/../../src/Util/ConsoleQueueRunner.php).

@totten
Copy link
Member Author

totten commented Jan 17, 2023

Yeah, I haven't quite figured out why -- but I bisected the changes, and it has something to do with the update to SqlTrait::createItem().

@totten
Copy link
Member Author

totten commented Jan 17, 2023

Fixed by telling executeQuery() skip multilingual rewrites. The serialized $task record was getting filtered, yielding an invalid record (which could not be unserialized). Ex:

  1. You have an upgrade-task with a title. The title includes a table name.
    • Update the foozballs in civicrm_dashboard for whimsy
  2. The upgrade-task is serialized. Part of this is the title with the table name.
    • s:52:"Update the foozballs in civicrm_dashboard for whimsy";
  3. The entire INSERT query was filtered by Schema::rewriteQuery()
    • s:52:"Update the foozballs in civicrm_dashboard_en_US for whimsy"; (string is now 57 chars but described as 52 chars -- which is malformed/unloadable)

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Jan 17, 2023
@eileenmcnaughton
Copy link
Contributor

My general take is that the tests are passing & they are the most strict consumer of this code - but I have not actually merged at this stage in case @seamuslee001 wants to weigh in

@eileenmcnaughton
Copy link
Contributor

I'm gonna merge this now - cos all these failing tests are making stuff hard - I'm sure @seamuslee001 can add post-merge comments if need be

@eileenmcnaughton eileenmcnaughton merged commit ca6f596 into civicrm:master Jan 17, 2023
@totten totten deleted the master-queue-tz branch January 17, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants