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

Start using apiv4 in test setup #17020

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 8, 2020

Overview

This is a tidy up PR but it strikes me we should be migrating code that does set up for tests to apiv4 & I'm wondering how that should look - do we want helpers or should it just look like the change in line 52 here. I feel like the main thing that might be missing from the helpers is that we ensured debug output was on in the helpers & somewhat improved test fail messaging

Before

After

Technical Details

@colemanw @seamuslee001 thoughts - specifically on this line https://github.com/civicrm/civicrm-core/pull/17020/files#diff-5c38127f768355a7d41757435987de99R52

Comments

@civibot
Copy link

civibot bot commented Apr 8, 2020

(Standard links)

@civibot civibot bot added the master label Apr 8, 2020
@@ -42,7 +49,7 @@ public function createCustomGroup($params = []) {
'max_multiple' => 0,
], $params);
$identifier = $params['name'] ?? $params['title'];
$this->ids['CustomGroup'][$identifier] = $this->callAPISuccess('CustomGroup', 'create', $params)['id'];
$this->ids['CustomGroup'][$identifier] = CustomGroup::create()->setValues($params)->execute()->first()['id'];
Copy link
Member

@totten totten Apr 8, 2020

Choose a reason for hiding this comment

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

(From MatterMost) I feel like we should be using apiv4 where we can now for everything other than testing apiv3 - but our reliance / comfort with helper functions makes that surprisingly hard

For this kind of thing - setting up a couple records - I agree that it makes more sense to start using APIv4 instead of APIv3. +1

IMHO, it's nice that civicrm_api4() doesn't seem to wrap-up exceptions in the same way that civicrm_api3() does. So I don't think we need an analog for callAPISuccess()?

Regarding test-helpers generally... it's not monolithic IMHO. There probably are some that could be replaced with API calls. But consider a helper like individualCreate() or participantCreate() which bakes-in some defaults/examples. These are not meaningful for a general audience or runtime users, so it doesn't feel right to present them as a public "API". Never-the-less, they're useful in writing tests (because it otherwise gets quite redundant to define examples).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@totten I was imagining 'fixing' individualCreate to itself call apiv4 though

Copy link
Member

Choose a reason for hiding this comment

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

Yup, that makes sense. 👍

@colemanw
Copy link
Member

colemanw commented Apr 8, 2020

@eileenmcnaughton - style checker is strangely picky about some things.

@colemanw
Copy link
Member

colemanw commented Apr 8, 2020

@eileenmcnaughton I fixed the style thing and also added ->setCheckPermissions(FALSE). If we did add a wrapper around civicrm_api4 for the tests, disabling permission checks would be a sensible default to set in it.

@eileenmcnaughton eileenmcnaughton merged commit b8376fb into civicrm:master Apr 8, 2020
@eileenmcnaughton eileenmcnaughton deleted the dedupe1 branch April 8, 2020 21:05
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