[meta] Add additional performance test coverage to core

I do not like the strategy of rounding up getScriptBytes() and getStylesheetBytes() and then using assertLessThan().

What happens if some JS or CSS file gets a thorough cleaning and is 50% of the original size? The test continues to pass. Then, as new cruft accumulates, no one notices until it gets larger than it was when the test was first written.

Instead of something like this

    $this->assertLessThan(7500, $performance_data->getScriptBytes());

I suggest something like

    $this->assertEqualsWithDelta(7463, $performance_data->getScriptBytes(), 50.0);

When the number of bytes goes up or down by 50, we will have to update the test (or reject the change).

Reference: https://docs.phpunit.de/en/10.5/assertions.html#assertequalswithdelta

@benjifisher assertCountBetween() was originally added for a different problem (indeterminate query counts, which are mostly fixed in the existing coverage, cold cache issues in yesterday's demo notwithstanding), but I think that would be a great use case for it - definitely agreed we should add lower bounds so that we enforce updating the test when things improve.

There are a couple of things I do not like about the developer experience (DX).

When writing a test, I can use var_dump($performance_data) to find the current (baseline) values. Then I can copy the assertions from an existing test and update the values.

A little DX improvement would be to change the order of the assertions and/or the order of the PerformanceData class variables so that they match. Maybe declare the $queries variable first or last so that it does not interrupt the other variables in var_dump().

Maybe a better idea is to add a PerformanceData::toArray() method, returning a keyed array. Then we could use a single assertSame() or assertEquals() to compare the expected values to the actual values. When a test failed, it would show the keys as well as the (different) values. That would be an improvement over the current DX, which is something like this:

There was 1 failure:

1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPagePerformance
Failed asserting that 1 is identical to 3.

/var/www/html/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php:51
/var/www/html/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php:28

and then you have to look at Line 51 in the test class to figure out which metric changed.

An alternative would be to add an assertMetrics() method to PerformanceTestTrait. Something like this:

  public function assertMetrics(
    PerformanceData $performance_data,
    ?int $stylesheet_count = NULL,
    ?int $script_count = NULL,
    ?int $stylesheet_bytes = NULL,
    ?int $script_bytes = NULL,
    ?int $query_count = NULL,
    ?int $cache_get_count = NULL,
    ?int $cache_set_count = NULL,
    ?int $cache_delete_count = NULL,
    ?int $cache_tag_checksum_count = NULL,
    ?int $cache_tag_is_valid_count = NULL,
    ?int $cache_tag_invalidation_count = NULL,
  ) {
    // ...
  }

This method would make an assertion for any non-null argument, and use assertEqualsWithDelta() or assertCountBetween() for the byte counts. And we could add the optional $message parameter to each assertion so that a failing test tells you which metric is out of range.

BTW, this looks clunky to me:

  public function logQuery(string $query): void {
    $this->queries[] = $query;
    $this->queryCount++;
  }

Why not remove the $queryCount variable and

  public function getQueryCount(): int {
    return count($this->queries);
  }

An alternative would be to add an assertMetrics() method to PerformanceTestTrait

I really like this idea - could you open a new issue for it?

Done: 📌 Add PerformanceTestTrait::assertMetrics() so it is easier to write performance tests Active .