Problem/Motivation

From #3202016-30: Let GDToolkit support AVIF image format:

ToolkitGdTest::testManipulations() is a monster that dates back to simpletest when multiple tests were performed in a single method to avoid multiple installations. In the era of Kernel tests, it's better split it in more understandable bits.

This entire class dates back to Simpletest times, today we can use data providers and have more atomic tests.

Steps to reproduce

n/a

Proposed resolution

Modernize the test.

Remaining tasks

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

CommentFileSizeAuthor
#37 interdiff_32-37.txt1.37 KBspokje
#37 3239935-37-d95.patch32.17 KBspokje
#32 3239935-32-d95.patch32.03 KBspokje

Issue fork drupal-3239935

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

mondrake created an issue. See original summary.

mondrake opened merge request !1274

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented

Quickly skimmed the diff and found only one nit

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks for the review @andypost! Comment and changes in the MR.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

I find it ready for commiters! great clean-up!

daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Needs work

Created 2 unresolved threads on the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Good points @daffie, thank you

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Rerolled

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

This look nicer but there's a massive difference in test performance. What used to take 8 secs is now taking over 2 mins and making my fans blow very very hard. Perhaps we can refactor the test to split things up but not use data providers because ktb site installation is not free.

Compare:

PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.21
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
....                                                                4 / 4 (100%)

Time: 7.96 seconds, Memory: 8.00 MB

OK (4 tests, 461 assertions)

to

PHPUnit 8.5.21 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.21
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
................................................................. 65 / 93 ( 69%)
............................                                      93 / 93 (100%)

Time: 2.15 minutes, Memory: 8.00 MB

OK (93 tests, 1260 assertions)

  • alexpott committed d0c712a on 9.3.x 
    Issue #3239935 by mondrake, andypost, daffie: Refactor ToolkitGdTest

  • alexpott committed 1ac9aaf on 9.3.x 
    Revert "Issue #3239935 by mondrake, andypost, daffie: Refactor...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Oops did not mean to commit this one given #11. Sorry for the noise.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

While trying to introduce AVIF image processing in #3202016: Let GDToolkit support AVIF image format, with the current test in HEAD I got

PHPUnit 9.5.8 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
ERS.                                                                4 / 4 (100%).

Time: 00:03.728, Memory: 6.00 MB

There was 1 error:

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

--

There was 1 risky test:

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations
This test did not perform any assertions

/var/www/html/core/tests/Drupal/Tests/Listeners/DrupalListener.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:446
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:376
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:677
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

ERRORS!
Tests: 4, Assertions: 7, Errors: 1, Skipped: 1, Risky: 1.

We only know from this this that, somewhere, a segfault occurred. With an equivalent of the patch here, I got

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\KernelTests\Core\Image\ToolkitGdTest
............E..............E..............E..............E....... 65 / 96 ( 67%)
.......E...EEEEEEE....EEEEE.S..                                   96 / 96 (100%)

Time: 01:01.873, Memory: 6.00 MB

There were 17 errors:

1) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #11 ('image-test.png', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

2) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #26 ('image-test.gif', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

3) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #41 ('image-test-no-transparency.gif', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

4) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #56 ('image-test.jpg', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

5) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #71 ('img-test.webp', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

6) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #75 ('img-test.avif', 'resize', array('resize', array(20, 10), 20, 10, array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

7) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #76 ('img-test.avif', 'scale_x', array('scale', array(20), 20, 10, array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

8) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #77 ('img-test.avif', 'scale_y', array('scale', array(10), 20, 10, array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

9) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #78 ('img-test.avif', 'upscale_x', array('scale', array(80, true), 80, 40, array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

10) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #79 ('img-test.avif', 'upscale_y', array('scale', array(40, true), 80, 40, array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

11) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #80 ('img-test.avif', 'crop', array('crop', array(12, 4, 16, 12), 16, 12, array(array(255, 255, 255, 0), array(255, 255, 255, 0), array(255, 255, 255, 0), array(255, 255, 255, 0))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

12) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #81 ('img-test.avif', 'scale_and_crop', array('scale_and_crop', array(10, 8), 10, 8, array(array(0, 0, 0, 0), array(0, 0, 0, 0), array(0, 0, 0, 0), array(0, 0, 0, 0))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

13) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #86 ('img-test.avif', 'convert_avif', array('convert', 40, 20, array('avif'), array(array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0), array(0, 0, 0, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

14) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #87 ('img-test.avif', 'rotate_90', array('rotate', array(90, '#FF00FF'), 20, 40, array(array(0, 0, 0, 127), array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

15) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #88 ('img-test.avif', 'rotate_transparent_90', array('rotate', array(90), 20, 40, array(array(0, 0, 0, 127), array(255, 0, 0, 0), array(0, 255, 0, 0), array(0, 0, 255, 0))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

16) Drupal\KernelTests\Core\Image\ToolkitGdTest::testManipulations with data set #89 ('img-test.avif', 'desaturate', array('desaturate', array(), 20, 40, array(array(76, 76, 76, 0), array(149, 149, 149, 0), array(29, 29, 29, 0), array(225, 225, 225, 127))))
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

17) Drupal\KernelTests\Core\Image\ToolkitGdTest::testCreateImageFromScratch
PHPUnit\Framework\Exception: Segmentation fault

/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:270
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:187
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:678
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:670
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:143
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:96

ERRORS!
Tests: 96, Assertions: 1096, Errors: 17, Skipped: 1.

This tells us that any non-AVIF image converted to AVIF fails, any operation on an AVIF image fails, but converting an AVIF file to any other non-AVIF image succeeds. This helped to point out that the failure is only when saving an image to AVIF format, hence finding https://bugs.php.net/bug.php?id=81217.

To me, the decrease in perfomance of the test is overcome by the informational gain that having more atomic tests brings. Yes it is significant, but IMHO this is how it should be, and on the testbot a 1 minute long test is not a particularly long one.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Is there a way to run this tests in parallel, it could help minimize time...

Otherwise it should not be a blocker for AVIF format support into 9.5/10

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 9.5.x-dev » 10.0.x-dev
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Looks ready for me, thank you

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Just a few nits added to the MR review. Thanks for working on this @mondrake

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Also point #11 still stands - we're changing a test that takes 3.6 seconds locally for me to one that takes 81.1 seconds... like over 1 minute.

mondrake’s picture

mondrake
Location 🇮🇹
commented

In #15 I explained why I think we shoud take this hit IMHO

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

To fix #11 I'd drop the data provider for testManipulations... and change the provider to testManipulations() and change testManipulations() to doTestManipulations()... and call that from testManipulations(). FWIW the changes here, moving each operation into a separate test, lose coverage of the fact that operations can be chained in the same process without affecting each other.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Fixed the review points and removed pre-PHP8 checks, no longer relevant in D10.

FWIW the changes here, moving each operation into a separate test, lose coverage of the fact that operations can be chained in the same process without affecting each other.

Well, the current code in HEAD

https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/tests/Drupa...

    foreach ($files as $file) {
      foreach ($operations as $op => $values) {
        // Load up a fresh image.
        $image = $this->imageFactory->get('core/tests/fixtures/files/' . $file);

makes so that at every loop we always start from the source image, so we do not really chain ops on the current image as part of the loop - hence why I moved that to dataprovider.

I'd drop the data provider for testManipulations... and change the provider to testManipulations() and change testManipulations() to doTestManipulations()... and call that from testManipulations()

Sure, but that would mean that the first failing op in the loop will fail the entire test, whereas I value the independent testing of each file-operation combination - see #15

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All changes look good to me.
All points of @alexpott have been addressed.
Back to RTBC.

mondrake’s picture

mondrake
Location 🇮🇹
commented

rebased

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Okay I've work out why my local was making it take so long... I have some themes in my root/themes directory that was making things take a while. There are other things we can do to speed things up. One fix is #3293446: Create less static cache in ExtensionDiscovery during KernelTests

The test now goes from 3 seconds to 30 seconds - not so bad. Still a factor of 10 but I think there are other things we can do to improve test running time.

Committed and pushed fa8a8df587 to 10.1.x and 6ef26c6905 to 10.0.x. Thanks!

Can we get a 9.5.x / 9.4.x version of the patch.

  • alexpott committed fa8a8df on 10.1.x 
    Issue #3239935 by mondrake, alexpott, andypost, daffie: Refactor...

  • alexpott committed 6ef26c6 on 10.0.x 
    Issue #3239935 by mondrake, alexpott, andypost, daffie: Refactor...

mondrake closed merge request !1274

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3239935-32-d95.patch32.03 KB
spokje’s picture

spokje
Primary language Dutch
commented
Status: Patch (to be ported) » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented

The problem will be with PHP 7.3 that does not support typed class properties.

spokje’s picture

spokje
Primary language Dutch
commented

@mondrake Ah crap, thanks for being as sharp as a knife (as always), I always forgot about the stepchild PHP 7.3.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3239935-37-d95.patch32.17 KB
new interdiff_32-37.txt1.37 KB

Let's give this a whirl.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Thank you @Spokje!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 414b11760d to 9.5.x and 33e22391ec to 9.4.x. Thanks!

  • alexpott committed 414b117 on 9.5.x 
    Issue #3239935 by mondrake, Spokje, alexpott, andypost, daffie: Refactor...

  • alexpott committed 33e2239 on 9.4.x 
    Issue #3239935 by mondrake, Spokje, alexpott, andypost, daffie: Refactor...

Status: Fixed » Closed (fixed)

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