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) CiviUnitTestCase - Cleanup and simplify the DB-reset mechanism #25178

Merged
merged 10 commits into from
Dec 19, 2022

Conversation

totten
Copy link
Member

@totten totten commented Dec 15, 2022

Overview

Simplify the code which handles data-setup in CiviUnitTestCase. This removes several properties and functions which are not really used. Instead, callCiviEnvBuilder (which includes similar/better helpers -- and which is shared by Civi\Test::headless(), Civi\Test::e2e(), Api4TestBase, and most civix testing templates).

Before

Primarily, the functionality here is that setUpBeforeClass() and setUp() call _populateDB(),

There is a series of inter-related properties and functions which reference each other - but they are not used externally. These are:

class CiviUnitTestCase {
  protected $_dbconn;
  protected static $_dbName;
  public static $populateOnce;
  public function requireDBreset(): bool;
  public static function getDBName(): string;
  public function getConnection(): WhoKnowsWhat;
  protected function getDataSet(): WhoKnowsWhat;
  protected static function _populateDB(): bool;
}

After

Primarily, the functionality here is that setUpBeforeClass() and setUp() call buildEnvironment()->apply(TRUE),

All the "Before" methods are removed. Instead, there is one new, short method:

class CiviUnitTestCase ... {
  public static function buildEnvironment(): CiviEnvBuilder
}

Comments

The existing policy in CiviUnitTestCase have a very strong tendency to do DB resets. The PR preserves this policy.

Most of the commits are fairly basic: search for "XX", find "XX" is only used 0-1 times, and remove "XX".

The most interesting commit is probably 8cfba74 -- that's where it becomes apparent that the funny conditionals in _populateDB() don't serve a purpose, and that $DBResetRequired is inert (even though ~30 test-classes mention it).

This file has a huge amount of implicit test-coverage by way of ./tests/phpunit/ -- CiviUnitTestCase is extended by 450+ test-classes.

@civibot
Copy link

civibot bot commented Dec 15, 2022

(Standard links)

@civibot civibot bot added the master label Dec 15, 2022
@totten
Copy link
Member Author

totten commented Dec 15, 2022

civibot, test this please

1 similar comment
@totten
Copy link
Member Author

totten commented Dec 15, 2022

civibot, test this please

The property is initialized to FALSE. It seems that the idea was to let some classes
override this, but it is never used.
This function returns a connection that nobody uses.  It has only one
caller.  It's mostly interesting because it can reset the database.
Hiding it under the name `getConnection` is misleading.
In practice, this check is the first thing done by `setUpBeforeClass()`.
Might as well move it there.

Technically, this check is also called by way of `setUp()`, but it's
redundant at that point. (Constants are constants...)
@totten totten force-pushed the master-civiunitestcase branch from 371682c to 168ff84 Compare December 16, 2022 05:14
At this point, it's just a bunch of verbosely written conditions that
appear to be trueisms;

* When called via `setUpBeforeClass()`, the `$perClass` flag is TRUE,
  so it does the reset.
* When called via `setUp()`, the `$object` flag is non-null,
  so it does the reset.

At this point, the actual reset bit is 1-line. Easier to just call that.
@totten totten changed the title (EXP) (Do not merge) CiviUnitTestCase - Multiple cleanups CiviUnitTestCase - Cleanup and simplify the DB-reset mechanism Dec 16, 2022
@totten totten changed the title CiviUnitTestCase - Cleanup and simplify the DB-reset mechanism (REF) CiviUnitTestCase - Cleanup and simplify the DB-reset mechanism Dec 16, 2022
@totten
Copy link
Member Author

totten commented Dec 16, 2022

I've filled in the description and removed the "do not merge" flag. Tests are passing.

If you want to skim the patch, I suggest reading the "Before/After" summary - and then the individual commits. (The aggregated diff is a bit crisscrossy, but the individual ones are very bite-sized.)

@demeritcowboy demeritcowboy merged commit 0ef0fac into civicrm:master Dec 19, 2022
@totten totten deleted the master-civiunitestcase branch December 20, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants