dev/core#4088 Convert api4 helper functionality to a trait & make available #25412

eileenmcnaughton · 2023-01-24T21:53:21Z

Overview

Convert api4 helper functionality to a trait & make available

First - ask jenkins....

Before

What is the old user-interface or technical-contract (as appropriate)?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

After

What changed? What is new old user-interface or technical-contract?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.

Technical Details

If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.

Comments

Anything else you would like the reviewer to note

civibot · 2023-01-24T21:53:23Z

No issue was found matching the number given in the pull request title. Please check the issue number.

civibot · 2023-01-24T21:53:24Z

(Standard links)

If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

totten · 2023-01-24T22:15:23Z

Civi/Test/Api4TestTrait.php

+    unset($record);
+    $saved = civicrm_api4($entityName, 'save', $saveParams);
+    foreach ($saved as $item) {
+      $this->testRecords[] = [$entityName, [[$idField, '=', $item[$idField]]]];


Can this $testRecords property be extracted too?

as in setTestRecords & addTestRecord?

As in public $testRecords = []; or protected $testRecords = [];

hmm yeah ok - I'll add the getters & setters too if I'm going that

Actually - it can be private so I don't need them

eileenmcnaughton · 2023-01-24T22:20:50Z

Civi/Test/Api4TestTrait.php

+   * @param int $len
+   * @return string
+   */
+  protected function randomLetters(int $len = 10): string {


this is called from one child class - tbh I'd rather make it private & have that class do it's own thing

eileenmcnaughton · 2023-01-24T22:21:31Z

Civi/Test/Api4TestTrait.php

+   * @return array
+   * @throws \CRM_Core_Exception
+   */
+  public function getRequiredValuesToCreate(string $entity, array &$values = []): array {


this is protected as the Conformance calls it directly

we return $values - maybe we can not pass-by-ref as well....

totten · 2023-01-24T22:30:06Z

Civi/Test/Api4TestTrait.php

+ *
+ * @package Civi\Test
+ *
+ * This trait defines a number of helper functions for testing APIv4.


I'd find it easier to understand with some kind of pointer about how to use the class. (Which of the 8 helpers should I care about?) Maybe like:

Suggested change

* This trait defines a number of helper functions for testing APIv4.

* Define helpers for generating, tracking, and cleaning test-data with APIv4. Specifically:

* - `createTestRecord()` will autofill a new record with example data.

* - `deleteTestRecords()` will batch-delete all the test-data created by `createTestRecord()`

Yep - I think those might be the only ones we would encourage people to use so that's probably right - I'm trying to privatise as much else as I can

@totten I've updated this but what I found is there are 2 functions

createTestRecord()
saveTestRecords()

I suppose that's consistent with apiv4 but it feels like they should use the same verb - given it would be work to change I am VERY happy to be convinced otherwise

Yea, they both use the APIv4 save action, so changing to saveTestRecord() would be more consistent all-round.

seamuslee001 · 2023-01-24T22:44:01Z

@eileenmcnaughton once you have resolved the comments from Tim I would say this is MOP

eileenmcnaughton · 2023-01-24T22:46:58Z

@seamuslee001 thanks -

I got one fail - not sure if it is co-incidental or related - trying again

api\v4\Entity\ManagedEntityTest::testRevertSavedSearch
Failed asserting that '2023-01-24 22:33:06' is equal to '2023-01-24 22:33:07' or is greater than '2023-01-24 22:33:07'.

/home/jenkins/bknix-dfl/build/build-1/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Entity/ManagedEntityTest.php:119
/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:1721

eileenmcnaughton · 2023-01-25T00:40:21Z

Hmm - well I made it worse there! Fortunately local fails too so I'll find & fix

It was doing both pass-by-ref and return to give back the value - this settles on using return.

I'm not sure if the test fail was an intermittent or related but this seems cleaner

eileenmcnaughton · 2023-01-25T04:31:02Z

@seamuslee001 @totten it is passing now - the only hesitation I have is #25412 (comment) - but that is also ok for a follow up if we dither

colemanw · 2023-01-25T13:06:17Z

@eileenmcnaughton +1 on changing createTestRecord to saveTestRecord, but otherwise my only beef with this PR is that:

Before: saved test records would be automatically cleaned up during teardown; the author of the test doesn't have to think about it.
After: now the trait provides a cleanup function, but no guidance on how to use it.

I've seen a lot of unit-test authors struggle with the concept of cleanup. A lot of people try to clean stuff (if they do it at all) by putting some delete calls at the end of their test, which makes sense to a procedural thought-process but doesn't work for unit tests. I worry about people calling deleteTestRecords from within their tests, or forgetting to call it at all.
My recommendation would be to add a tearDown function to this trait. That will cause annoying collisions with any tearDown function implemented in a class using this trait, which IMO is a good thing because it forces the dev to think about cleanup! To resolve the conflict they can simply do:

use  Api4TestTrait {
  tearDown as testRecordTearDown;
}

public function tearDown() {
  $this->testRecordTearDown();
  // other stuff
}

eileenmcnaughton · 2023-01-25T19:43:53Z

@colemanw ok - I think both the rename & the tearDown fix are going to each involve touching quite a few classes so I think it would be helpful if we can merge this first

totten · 2023-01-26T00:02:39Z

Yeah, I agree the tear-down protocol is the fiddly thing. Let's verbalize some options:

Continue emphasizing Api4TestBase as the main way to use this. Do whatever you need to make it seem more official. Api4TestTrait is the more advanced tool (for someone wiring-up their own base-class).
Encourage test-classes to resolve function conflicts (use Api4TestTrait { tearDown as testRecordTearDown; }).
Move the cleanup into CiviTestListener. If a class includes Api4TestTrait, then automatically call deleteTestRecords().
Move the cleanup into CiviTestListener. Automatically call setup/teardown functions based on prefixes. (Any test-trait can define a tear down - eg Api4TestTrait::tearDown_api4()).

#1 is simplest but doesn't go very far to encourage trait decomposition. #2+#3+#4 would go further to encouraging traits, but they're more complicated. #2 puts the complexity on the test-authors. #3+#4 put the complexity on the trait-authors.

eileenmcnaughton · 2023-01-26T02:31:49Z

I took a look at another place the require_once is used and now I think we should rename the apiv4TestTrait - it really is just an EntityProviderTrait or something like that. There is nothing especially apiv4 about it. Given that it makes me think a bit differently - ie it's not an equivalent to the v3Trait

I also hit the fact that the test was using #25432 yet another class for managing custom fields - which feels like it should be combined with the more heavily used CRMTraits_Custom_CustomDataTrait and made generally available to extensions.

We have 10 traits in various stages of maturity in the tests directory - so I feel like we should be a bit clearer on what we are working towards than my initial 'this is like apiv3Trait' - although actually implementing feels out of scope of this round

civibot bot added the master label Jan 24, 2023

eileenmcnaughton force-pushed the api4trait branch 3 times, most recently from 2340aa8 to be7acd2 Compare January 24, 2023 22:13

totten reviewed Jan 24, 2023

View reviewed changes

Convert api4 helper functionality to a trait & make available

937d757

eileenmcnaughton force-pushed the api4trait branch from be7acd2 to 937d757 Compare January 24, 2023 22:19

eileenmcnaughton commented Jan 24, 2023

View reviewed changes

totten reviewed Jan 24, 2023

View reviewed changes

eileenmcnaughton force-pushed the api4trait branch from 500c7ff to 09b8d53 Compare January 24, 2023 22:47

eileenmcnaughton added 3 commits January 25, 2023 15:11

Clarify function signature

4ba2b93

It was doing both pass-by-ref and return to give back the value - this settles on using return.

Move over testRecords property

9fdf0f4

Use Trait directly from managed entity test

72d3d0e

I'm not sure if the test fail was an intermittent or related but this seems cleaner

eileenmcnaughton force-pushed the api4trait branch from 09b8d53 to 72d3d0e Compare January 25, 2023 02:11

Cleanup comments, remove unnecessary public function

34f8db5

colemanw merged commit 4e14bc4 into civicrm:master Jan 25, 2023

colemanw deleted the api4trait branch January 25, 2023 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#4088 Convert api4 helper functionality to a trait & make available #25412

dev/core#4088 Convert api4 helper functionality to a trait & make available #25412

eileenmcnaughton commented Jan 24, 2023

civibot bot commented Jan 24, 2023

civibot bot commented Jan 24, 2023

totten Jan 24, 2023

eileenmcnaughton Jan 24, 2023

totten Jan 24, 2023 •

edited

Loading

eileenmcnaughton Jan 24, 2023

eileenmcnaughton Jan 24, 2023

eileenmcnaughton Jan 24, 2023

eileenmcnaughton Jan 24, 2023

eileenmcnaughton Jan 24, 2023

totten Jan 24, 2023

eileenmcnaughton Jan 24, 2023

eileenmcnaughton Jan 25, 2023

colemanw Jan 25, 2023

seamuslee001 commented Jan 24, 2023

eileenmcnaughton commented Jan 24, 2023

eileenmcnaughton commented Jan 25, 2023

eileenmcnaughton commented Jan 25, 2023

colemanw commented Jan 25, 2023

eileenmcnaughton commented Jan 25, 2023

totten commented Jan 26, 2023 •

edited

Loading

eileenmcnaughton commented Jan 26, 2023

- * This trait defines a number of helper functions for testing APIv4.
+ * Define helpers for generating, tracking, and cleaning test-data with APIv4. Specifically:
+ * - `createTestRecord()` will autofill a new record with example data.
+ * - `deleteTestRecords()` will batch-delete all the test-data created by `createTestRecord()`

dev/core#4088 Convert api4 helper functionality to a trait & make available #25412

dev/core#4088 Convert api4 helper functionality to a trait & make available #25412

Conversation

eileenmcnaughton commented Jan 24, 2023

Overview

Before

After

Technical Details

Comments

civibot bot commented Jan 24, 2023

civibot bot commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totten Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jan 24, 2023

eileenmcnaughton commented Jan 24, 2023

eileenmcnaughton commented Jan 25, 2023

eileenmcnaughton commented Jan 25, 2023

colemanw commented Jan 25, 2023

eileenmcnaughton commented Jan 25, 2023

totten commented Jan 26, 2023 • edited Loading

eileenmcnaughton commented Jan 26, 2023

totten Jan 24, 2023 •

edited

Loading

totten commented Jan 26, 2023 •

edited

Loading