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

CiviUnitTestCase (etal) - Resolve spooky interaction #25855

Merged
merged 7 commits into from
Mar 21, 2023

Conversation

totten
Copy link
Member

@totten totten commented Mar 17, 2023

Overview

In #25673, there was a spooky interaction between tests -- which originated in a subtle detail of the setUp() process.

That PR includes the minimal change. This PR aims to be more proactive -- normalizing the setUp() and (hopefully) preventing similar spookiness in the future.

Before

Four tests in api_v3_* use this uncommon formulation:

   protected function setUp(): void {
     $this->useTransaction(TRUE);
     parent::setUp();
   }

After

The tests now match their 54 siblings in api_v3_* which use:

   protected function setUp(): void {
     parent::setUp();
     $this->useTransaction(TRUE);
   }

Anything with a flipped sequence will raise an error.

Comments

I only looked at tests/phpunit/api/v3. The testbot should now complain about similar issues in other suites.

totten added 2 commits March 16, 2023 21:02
This fixes an issue observed in review of 25673. With that patch included,
`JobProcessMailingTest` would still pass... but only in isolation. When
combined with `ExceptionTest`, it would fail.

I tried disabling the entire substance of `ExceptionTest`, and it still
failed. Say what?

The problem was _the order_ in which `ExceptionTest` does its initialization
(`parent::setUp()` and `$this->useTransaction()`). I suppose that order
determines the effectiveness of the transaction.

Most tests call `setup()` then `useTransaction()`. I looked for
outliers which flipped the order -- and found `EntityTagACLTest`
has the same init and the same spooky interaction.

After fixing the outliers, I'm getting the expected results on
`JobProcessMailingTest`.
@civibot
Copy link

civibot bot commented Mar 17, 2023

(Standard links)

@civibot civibot bot added the master label Mar 17, 2023
@totten totten changed the title CiviUnitTestCase (etal) - Resolve spooky interactions. CiviUnitTestCase (etal) - Resolve spooky interaction Mar 17, 2023
@eileenmcnaughton
Copy link
Contributor

Sounds like it might fail a few times before it passes but good to merge when it does

@totten
Copy link
Member Author

totten commented Mar 17, 2023

After looking at the other suites, the notion of "common" and "uncommon" changed, but (so far) not enough to change the approach.

Suite Tends to use Comment
api3 setUp()=>useTransaction() Very strong trend
CRM useTransaction()=>setUp() Moderately strong trend
Civi Both 50/50 split
api4 TransactionalInterface Very strong trend. Internally, it looks analogous to setUp()=>useTransaction(), but the names are different.

Going literally and by pure numbers, setUp()=>useTransaction() has the edge. If you count TransactionalInterface as its behavioral cousin, then it's clearly the more common behavior.

(Full transparency: I don't have a sense for either order being intrinsically better than the other. But standardizing should make things more predictable/consistent, and it should be easier to standardize on the behavior that's more common. 🤞 And it fixed the flakiness on the locking PR.)

@eileenmcnaughton
Copy link
Contributor

Yeah - I don't care either way - I generally find the useTransaction kinda hard to work with (cos I can't check the database while stepping through it) - but am agnostic about the order of those 2 statements where we are using it

@totten
Copy link
Member Author

totten commented Mar 17, 2023

I'm gonna downgrade this to "merge ready". We want to make sure that #25673 passes for the right reasons, and it might be best to merge this after that one is settled.

@totten totten added merge ready PR will be merged after a few days if there are no objections and removed merge on pass labels Mar 17, 2023
@totten totten merged commit c1db7c7 into civicrm:master Mar 21, 2023
@totten totten deleted the setup-order branch March 21, 2023 03:55
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.

2 participants