Add a flag to KernelTestBase to set up test environment only once per test class in setUpBeforeClass()

Created on 21 August 2023, about 1 year ago
Updated 7 January 2024, 8 months ago

Problem/Motivation

Some kernel tests could be sped up by installing Drupal only once per test class, rather than for every test method.

Steps to reproduce

Proposed resolution

Add a flag to KernelTestBase which causes the installation to be done in setUpBeforeClass() instead of setUp().

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Feature request
Status

Active

Version

11.0 🔥

Component
PHPUnit 

Last updated 1 minute ago

Created by

🇬🇧United Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @joachim
  • 🇬🇧United Kingdom joachim

    Well, this is turning out to be way more complicated than I anticipated, because all the helper methods called by setUp() set properties on the test object, e.g.:

        $this->siteDirectory = $test_db->getTestSitePath();
    

    For all of these to be called by setUpBeforeClass(), they can't do that, because setUpBeforeClass() is a static method.

    It's going to be complicated to unpick, but I think potentially beneficial - there is a mishmash of methods that return things and method that set properties. I've already found one thing that doesn't look right:

    bootEnvironment() calls $this->getDatabaseConnectionInfo() twice, and $this->getDatabaseConnectionInfo() makes use of $this->databasePrefix, but during the first call, $this->databasePrefix is not yet initialised!

  • 🇺🇸United States Mile23 Seattle, WA

    Maybe make issues about the db and prefix stuff if it's worrisome, and soft-postpone this on that...?

  • 🇬🇧United Kingdom joachim

    Agreed -- maybe several small issues that refactor the setUp() helpers.

  • 🇷🇺Russia Chi

    KernelTestBase needs deep refactoring anyway. I think it's worth to create a new base class for kernel tests. Also $runTestInSeparateProcess needs to be disabled as it will case setUpBeforeClass and tearDownAfterClass hooks run after each test method.

  • 🇬🇧United Kingdom joachim

    I discussed this with @Alex Pott at DrupalCon Lille a few months ago.

    His comment IIRC was along the lines of 'you can usually just comment parts out of the code'.

    Here's an example where AFAICT, you can't. I want to test all the possible combinations of 4 factors. So I'm using a cross product setup. To do so with a @dataProvider would mean a kernel test setup for each combination, which is needlessly expensive.

    So I'm doing the cross product manually in the test method like this:

        foreach ([FALSE, TRUE] as $permission_access) {
          foreach ([FALSE, TRUE] as $operand_access) {
            foreach ([FALSE, TRUE] as $operability) {
              foreach ([FALSE, TRUE] as $reachable) {
                $this->state->set('test_mocked_control:permission_access', $permission_access ? AccessResult::allowed() : AccessResult::forbidden());
                $this->state->set('test_mocked_control:operand_access', $operand_access ? AccessResult::allowed() : AccessResult::forbidden());
                $this->state->set('test_mocked_control:operability', $operability);
                $this->state->set('test_mocked_control:next_state', $reachable ? 'cake' : NULL);
    
                // Generate the links.
                $links = $action_link->getStateActionPlugin()->buildLinkArray($action_link, $user_no_access);
    
                // No access always means no links.
                if (!$permission_access || !$operand_access()) {
                  $this->assertEmpty($links, implode(':', [$permission_access, $operand_access, $operability, $reachable]));
                }
    

    But this has a number of drawbacks:

    - I have to ensure each assertion has a custom message otherwise I will have no idea which set of values caused a failure
    - furthermore, I have to use cross product values which are easily stringable, so the message can be easily made without complex code. This harms DX, as it would have been easier to read if the values for the access factors were AccessResult objects -- the checks further down would be easier to read than the boolean check for instance
    - If there are any failures, I only see one. With a data provider, I get a complete overview of what's wrong with the system
    - I can't run just a single combination for debugging. Or well, I can if I rewrite the cross product code like this so that values can be commented out, but it still requires a lot of faffing about to isolate a single case:

        $permission_access_values = [
          FALSE,
          TRUE,
        ];
        $operand_access_values = [
          FALSE,
          TRUE,
        ];
        $operability_values = [
          FALSE,
          TRUE,
        ];
        $reachable_values = [
          FALSE,
          TRUE,
        ];
    
        foreach ($permission_access_values as $permission_access) {
          foreach ($operand_access_values as $operand_access) {
            foreach ($operability_values as $operability) {
              foreach ($reachable_values as $reachable) {
    

    All of this is reinventing the wheel which PHPUnit provides with the data provider system.

  • 🇧🇪Belgium attiks

    We ran into the same issue, and converted our code to do something similar but using a flag inlineDataProvider on the base class, so whenever needed we can easily switch back to the normal behavior.

    Taken from https://github.com/UN-OCHA/un_date/blob/main/tests/src/Kernel/DateRender...

      /**
       * Test with UTC timezone.
       *
       * @x-dataProvider providerTestDataUtc
       * @x-dataProvider providerTestDataRandom
       */
      public function testDateRangeUtc($expected = NULL, $start = NULL, $end = NULL, $timezone = NULL) {
        if ($this->inlineDataProvider) {
          $data = array_merge(
            $this->providerTestDataUtc(),
            $this->providerTestDataRandom(),
          );
    
          foreach ($data as $name => $row) {
            $expected = $row['expected'];
            $start = $row['start'];
            $end = $row['end'];
            $timezone = $row['timezone'];
    
            // assertions        
          }
        }
        else {
          // assertions        
        }
    

    I also looked into changing the code, but it ain't straightforward since everything has to be static

  • 🇬🇧United Kingdom joachim

    Yeah, the big problem is that PHPUnit's once-per-class code is all static, and that's now how KernelTestBase is written.

    One potential avenue is the TestDatabase class -- I filed an issue to clean it up, then spoke to @AlexPott about it at DrupalCon Lille where it turned out I'd completely misunderstood the purpose of the class because it's badly named (his words :). It's more of a 'holder of stuff about the test site'. So we could move test site setup stuff to that (and also rename it!)

    Another avenue would be to have a testing kernel.

    In the meantime, I've refactored your approach to hybrid tests a bit so that the assertions code is DRY:

      /**
       * Data provider
       */
      public function dataProvider() {
        return [
          'alpha' => [1, 2],
          'beta' => [3, 4],
        ];
      }
    
      /**
       * For development, restore the annotation below and comment out the one
       * on the quickForCI() method.
       *
       * @NOTtest
       * @dataProvider dataProvider
       */
      public function expensiveForUseInDevelopment(...$parameters) {
        $this->doTest(...$parameters);
      }
    
      /**
       * @test
       */
      public function quickForCI() {
        foreach ($this->dataProvider() as $data_set) {
          $this->doTest(...$data_set);
        }
      }
    
      /**
       * Test the something thing.
       *
       * @param [type] $one
       * @param [type] $two
       */
      protected function doTest($one, $two) {
        $this->assertIsNumeric($one);
      }
    
Production build 0.71.5 2024