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

[NFC/Unit Test] dev/core#2385 - Add test for getCaseActivityQuery #19609

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

demeritcowboy
Copy link
Contributor

Overview

Adds some tests which are useful regardless of what happens with https://lab.civicrm.org/dev/core/-/issues/2385

Before

Some tests

After

Some tests

Technical Details

It's not comprehensive but adds some piece of mind.

Comments

Is Test.

@civibot
Copy link

civibot bot commented Feb 16, 2021

(Standard links)

@civibot civibot bot added the master label Feb 16, 2021
@demeritcowboy
Copy link
Contributor Author

Expression #1 of ORDER BY clause is not in GROUP BY clause and contains nonaggregated column 't_act.id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by

Oh right, I had turned full-group-by off locally to work on this. Well, at least it validates the code comments in the test.

@demeritcowboy
Copy link
Contributor Author

So I've adjusted to make it at least work with mysql 5.7 as on the test site here. There's 6 other tests in this file that fail on mariadb with full group by so I'm hesitant to add another, but looking at the edge tests it seems mysql 8 is the same as 5.7. 🤷

@demeritcowboy
Copy link
Contributor Author

Yes thanks. I'd say there's 3 angles here:

  • As you've noted, the unit tests run on a more lenient setup than mariadb setups (which then fail when you turn on full group by).
  • The right thing to do is fix the problem query, but that's hard since some of its performance improvements over the years have come from taking advantage of mysql and even specific versions of mysql.
  • My goal was simply to shore up some tests so that when I propose to remove the db view and copy it to just plain inline sql there'll be some added piece of mind. The pending patch commented in the lab ticket also passes all the tests in this file including this test if you either turn off full group by or run it on mysql 5.7.

So the 2nd is out of scope and the 3rd seems ok, but it's reinforcing the leniency in the 1st one.

Full-group-by issues always remind me of the MovieFone Seinfeld episode - "Why don't you just tell me the name of the movie you want to see?"

@eileenmcnaughton eileenmcnaughton merged commit 9d815b6 into civicrm:master Feb 21, 2021
@demeritcowboy demeritcowboy deleted the case-views branch February 21, 2021 14:11
@demeritcowboy
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants