Fix miscoordination between CiviUnitTestCase/CiviEnvBuilder. Fix flaky ConformanceTest. #25177
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tweaking the `$name` is theoretically useful if you have entirely independent data-sets that happen to live in the same DB. In practice, we only use CiviEnvBuilder to manage Civi-related data-sets.
Background
----------
Recall that `CiviUnitTestCase` and `CiviEnvBuilder` are two different
implementations of a similar concept.
* `Concept`: If two tests use the same baseline DB/environment, and if they
preserve the baseline, then you don't need to reset everything in between
tests. But if they change, then you need to reset.
* `CiviUnitTestCase`: The older rendition. Tightly coupled. Only allows
one specific baseline. Mingled with a lot of unrelated functionality in
`CiviUnitTestCase`.
* `CiviEnvBuilder`: The newer rendition. Loosely coupled. Allows different
baselines. Can be mixed into any plain-old `TestCase`. Used by
`Civi\Test::headless()` and `Civi\Test::e2e()`.
Problem Scenario
----------------
Suppose you have a mix of different tests running with the same DB -- e.g.
1. Run some tests based on `CiviEnvBuilder`
2. Run some tests based on `CiviUnitTestCase`
3. Run some tests based on `CiviEnvBuilder`
This wasn't originally anticipated, but it can happen -- either because
the test-suite is large+mixed, or because a developer is manually
running specific tests (which happen to be written differently).
The problem goes like this:
1. Run some tests based on `CiviEnvBuilder`
* This resets the DB and also stores a DB signature. ("Here is how
we setup the DB...")
2. Run some tests based on `CiviUnitTestCase`
* This resets the DB, but leaves the old DB signature in place.
3. Run some tests based on `CiviEnvBuilder`
* This sees the old DB signature and wrongly concludes that we
still have the DB from step (1).
Solution
--------
Whenever one resets the DB, it should update the DB signature.
|
(Standard links)
|
|
civibot, test this please |
1 similar comment
|
civibot, test this please |
|
@totten test fails seem to relate |
|
OK, those test-fails are a good thing -- this means that Of course, I guess I need to fix before we merge this. 👀 |
totten
commented
Dec 16, 2022
| @@ -54,6 +54,11 @@ public function on_civi_api_prepare(\Civi\API\Event\PrepareEvent $event) { | |||
| } | |||
| } | |||
|
|
|||
| public function setUpHeadless() { | |||
| // TODO: search_kit should probably be part of the 'headless()' baseline. | |||
| return \Civi\Test::headless()->install(['org.civicrm.search_kit'])->apply(); | |||
There was a problem hiding this comment.
This should work for getting 5.57 to a passing state. For master, we might move this to the parent (Api4TestBase::setUpHeadless()) or revise the definition of headless().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
(Replaces #25159. Follow-up to #24739.)
This fixes a subtle and general leak in how
CiviUnitTestCaseandCiviEnvBuildercoordinate the use of the headless DB. It was discovered while investigating a flaky test-result that stems from 5.57's #24739 -- specifically, there is a spooky interaction betweenCRM_Contribute_Import_Parser_ContributionTestandapi\v4\Entity\ConformanceTest-- where the outcome ofConformanceTestappears to change arbitrarily.After fixing the general leak, the spooky-interaction goes away, and the tests behave reliably. However, the reliable behavior is to complain. So I also update
ConformanceTest-- resolving the complaint, removing an adhoc override, and getting it to pass.(I suspect that developers have been locally+intermittently bitten by this spooky interaction in the past -- just not in a way that was visible to CI. So I'm including a bit more of write-up to explain what was fixed.)
General: Background
Recall that
CiviUnitTestCaseandCiviEnvBuilderprovide two different implementations of a similar concept.Basic Concept: When running tests, you need to setup the DB. You should do it regularly and reproducibly, but you shouldn't do it for every test-method (because it's slow/expensive). You should have some rules to decide (a) how to setup the DB and (b) when to reset it.
CiviUnitTestCase: The older implementation. Tightly coupled. Only allows one specific baseline. Mingled with a lot of unrelated functionality inCiviUnitTestCase.CiviEnvBuilder: The newer implementation. Loosely coupled. Allows different baselines. Can be mixed into any plain-oldTestCase. Used byCivi\Test::headless()andCivi\Test::e2e().CiviEnvBuilder, it resets the DB and stores a flag (civitests_rev) that says "Here is how I setup the DB." Each time you call again, it checks that flag and decides whether the current environment is good enough. (If not, then it resets.)The problem arises when you interleave test-runs that are based on the two systems. This could be a structural thing (one large test-suite with heterogeneous tests), or it could be circumstantial -- where a developer or testbot happens to run tests in a disagreeable order.
General: Before
When running interleaved tests, it misses some resets.
CiviEnvBuilderCiviUnitTestCaseCiviEnvBuilderGeneral: After
CiviEnvBuilderCiviUnitTestCaseCiviEnvBuilderSpecific: Before
CRM_Contribute_Import_Parser_ContributionTest(based onCiviUnitTestCase) enables a series of several extensions, includingsearch_kit. Prior to #24739, it cleaned up after itself (disabling+uninstalling each extension -- returning to CiviUnitTestCase's baseline environment). However, #24739 made it impossible to directly disablesearch_kit. Thus,ContributionTestleft SK active.ConformanceTest(based onCiviEnvBuilder) sometimes runs afterContributionTest(as inCiviCRM-Core-PR) -- which means SK is sometimes active. But it sometimes runs independently (as inCiviCRM-Core-Matrix) -- which means that SK is sometimes inactive.There is an override in
ConformanceTest. If you keep it, thenCiviCRM-Core-Matrixfails. If you remove it, thenCiviCRM-Core-PRfails.Specifics: After
It doesn't matter if you run
ConformanceTestviaCiviCRM-Core-PRorCiviCRM-Core-Matrix-- they both start from the same baseline (ie SK disabled).The override can be removed.
Specifics: Future
Given that policy has changed (
search_kitis becoming mandatory), it may be appealing to change the test-baselines -- so thatCiviUnitTestCaseandCiviEnvBuilderboth include it. Right now, I'm mostly concerned about getting a stable signal from the tests. Changes to baseline should be pursued as a separate exercise.