Problem/Motivation

While looking at #3567610: Adopt Core's stream wrapper, I realized that their could be some performance optimisation in the tests, for example in

tests/src/Kernel/PropTypeNormalization/BooleanPropTypeTest.php:

  /**
   * Provides data for testNormalization.
   */
  public static function normalizationTests() : array {
    return [
      "null value" => [NULL, NULL],
      "false value" => [FALSE, FALSE],
      "true value" => [TRUE, TRUE],
      "integer 0" => [0, FALSE],
      "integer pos" => [22, TRUE],
      "string empty" => ["", FALSE],
      "string zero" => ["0", FALSE],
      "string not zero" => ["22", TRUE],
      "html" => ["<p>0</p>", TRUE],
      "markup 0" => [["#markup" => "0"], FALSE],
      "markup 1" => [["#markup" => "1"], TRUE],
    ];
  }

Do not use data provider for kernel and functional tests. Data provider are nice for unit tests, but starting from kernel (and especially functional) tests, speed could be improved by not using it to avoid Drupal setup at each test case.

Proposed resolution

Add for each into the tests directly instead of using a data provider. Or call the data provider method in the tests and not as a "real" data provider.

Issue fork ui_patterns-3605477

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

grimreaper created an issue. See original summary.

pdureau’s picture

pdureau commented
Title: Refactor kernel tests » Better tests performance by avoiding data providers
enomo103’s picture

enomo103 commented
Assigned: Unassigned » enomo103

enomo103 opened merge request !517

enomo103’s picture

enomo103 commented
Assigned: enomo103 » Unassigned
Status: Active » Needs review
just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Status: Needs review » Reviewed & tested by the community

let's merge this proposition, we will discuss that later this morning

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hello,

Thanks for the MR and for providing KPIs about test execution time in the MR summary.

What is strange is that on the MR phpunit job: https://git.drupalcode.org/issue/ui_patterns-3605477/-/jobs/10523904

Time: 09:10.202, Memory: 1.22 GB
OK, but there were issues!
Tests: 144, Assertions: 5262, PHPUnit Deprecations: 174.

Compared with the latest pipeline of 2.0.x phpunit job: https://git.drupalcode.org/project/ui_patterns/-/jobs/10408958

Time: 08:45.768, Memory: 1.18 GB
OK, but there were issues!
Tests: 295, Assertions: 6289, PHPUnit Deprecations: 895.

I think it is normal that the number of tests is less because when using data providers each case is counted as an individual test. And also less PHPUnit deprecations which are triggered during setup I guess.

But I am surprised that there are less assertions and that the execution time is higher.

For the execution time, maybe it is due to CI?

PS: about assertions number, maybe it is due to the fact that there are assertions during the setup.

pdureau’s picture

pdureau commented

Thanks everybody. What about the functional tests?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

I searched for dataProvider in the codebase and it is used for unit and kernel tests.

So MR is ok.

mogtofu33 made their first commit to this issue’s fork.

mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » mogtofu33
Status: Reviewed & tested by the community » Needs work

An other improvement could be to switch to parallel execution with Drupal runner.

But to do so we need to fix some tests namespace, group and coding standards.
In the meantime let's move to PHPUnit #attributes to be ready for the future!

As it's becoming big, moved to an other branch.

mogtofu33 opened merge request !518

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Needs work » Needs review

Looking good, with parallel tests we go from 9 minutes 56 seconds to 2 minutes 18 seconds!

The phpstan issue is on deprecated code, what's our moto for that?

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: Unassigned » grimreaper

  • grimreaper committed 21960481 on 2.0.x authored by enomo103 
    refactor: #3605477 Better tests performance by avoiding data providers...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

The deprecation detected by PHPStan in modules/ui_patterns_field_formatters/tests/src/Kernel/Source/FieldFormatterSourceTest.php made me think, I was expecting to see a tests/src/Kernel/Source/FieldFormatterSourceTest.php but that's not the case.

Putting a comment to not forget, when deprecating modules/ui_patterns_field_formatters/src/Plugin/UiPatterns/Source/FieldFormatterSource.php, as the test been moved to the main module too.

I finish for this work session, need to check.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Discussed with @pdureau, effectively from commit https://git.drupalcode.org/project/ui_patterns/-/commit/f4fb6a8e27da0a17... the tests:

- modules/ui_patterns_field_formatters/tests/src/Kernel/Source/FieldFormatterSourceTest.php
- modules/ui_patterns_field_formatters/tests/src/Kernel/Source/FieldLabelSourceTest.php

had not been moved to the main module.

I will do that.

  • grimreaper committed 42f04511 on 2.0.x authored by mogtofu33 
    test: #3605477 add ci phpunit concurrent, fix tests attributes, style...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.