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

[REF] Fix utf8mb4 test in APIv4 and re-enable the altering of databas… #21001

Merged
merged 1 commit into from
Aug 7, 2021

Conversation

seamuslee001
Copy link
Contributor

…e in the v3 version of the test and fix handling for first name field

Overview

This is a follow on to #20905 which improves the APIv3 version of the test to do the faking of the database to test the re-writing of the query. It also properly fixes the handling for the v3 unit test.

Before

APIv4 unit test fails

After

V4 and V3 unit tests pass in a similar manner

ping @demeritcowboy @eileenmcnaughton

@civibot
Copy link

civibot bot commented Aug 2, 2021

(Standard links)

@civibot civibot bot added the master label Aug 2, 2021
@@ -5196,11 +5196,27 @@ protected function doMerge(bool $isReverse = FALSE) {
* out when we are.
*/
public function testEmojiInWhereClause(): void {
$schemaNeedsAlter = \CRM_Core_BAO_SchemaHandler::databaseSupportsUTF8MB4();
if ($schemaNeedsAlter) {
CRM_Core_DAO::executeQuery("
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 so my conclusion is we can't do this alter - I ended up ripping it out because I hit so many weird situations in various parts of the suite. I think in the end it's better to just let this test actually test non-utf8 on those matrix machines that don't support utf8mb4 rather than try to mock it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth noting - this used to not cause problems because \CRM_Core_BAO_SchemaHandler::databaseSupportsUTF8MB4() used to always incorrectly return FALSE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the exact same alter that is happening in the v4 version of this test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 yeah - which I think we should remove to get rid of bizarre fails like #20984

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton that is only failing because the Civi::statics cache hasn't been reset after the alter. This value https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/BAO/SchemaHandler.php#L922 with the current test setup will still return TRUE even tho table has now changed to not support emojis hence my reseting of the civi statics in the apiv4 test which is all that is needed to fix it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The apiv4 suite should probably unset Civi::statics like the main suite does -

or we will keep getting weirdness

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Aug 3, 2021

Yep - I remember this part of the process

api_v3_SyntaxConformanceTest::testCreateSingleValueAlter with data set #2 ('ActionSchedule')
api result array comparison failed checking if custom_6 was correctly updated
Array
(
    [update-params] => Array
        (
            [id] => 40
            [custom_6] => defaultValue
        )

    [update-result] => Array
        (

I can't recall the underlying issue for those fails - - but it is to do with the full text indexes in some way

@seamuslee001 seamuslee001 force-pushed the fix_utf8mb4_test branch 2 times, most recently from 434b012 to 747f2b6 Compare August 4, 2021 03:25
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 if you can win this fight then go ahead & merge :-)

@eileenmcnaughton
Copy link
Contributor

Yeah - this is about the point I gave up & decided that we should only test lack-of-support-for-utf8mb4 on databases that don't support utf8mb4 :-)

Stacktrace
api_v3_SyntaxConformanceTest::testCustomDataGet with data set #0 ('Acl')
incorrect count returned from Acl getcount
Failed asserting that 0 matches expected 1.

/home/jenkins/bknix-dfl/build/core-21001-6c1u3/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php:199
/home/jenkins/bknix-dfl/build/core-21001-6c1u3/web/sites/all/modules/civicrm/tests/phpunit/api/v3/SyntaxConformanceTest.php:917
/home/jenkins/bknix-dfl/build/core-21001-6c1u3/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:258
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

…e in the v3 version of the test and fix handling for first name field
@seamuslee001 seamuslee001 changed the base branch from master to 5.41 August 7, 2021 04:21
@civibot civibot bot added 5.41 and removed master labels Aug 7, 2021
@seamuslee001 seamuslee001 merged commit 586db1c into civicrm:5.41 Aug 7, 2021
@seamuslee001 seamuslee001 deleted the fix_utf8mb4_test branch August 7, 2021 04:46
@eileenmcnaughton
Copy link
Contributor

woot - sheer determination - take that jenkins

@demeritcowboy
Copy link
Contributor

well done

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