WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 11 days ago

#50640 accepted task (blessed)

[PHPUnit] Mark test as skipped instead of failing if GD extension is not availble

Reported by: ayeshrajans Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch php8
Focuses: Cc:

Description

There are some tests in WordPress test suite that depend on PHP gd extension. Some of these tests mark the test as a failure if the extension is not available in the system under test.

I believe these tests should be marked as skipped as opposed to marking as a failure, which causes some tests to fail (instead of skipping) in certain PHP 8 builds: #50639

This ticket is not related to PHP 8, but rather on the semantics of the tests.

Attachments (3)

50640-1.patch (4.2 KB) - added by ayeshrajans 4 months ago.
50640-composer.patch (1.2 KB) - added by ayeshrajans 9 days ago.
Patch to add the composer.json entry, and a circuit-breaker in bootstrap file. It has an array of requirements, and shows a if any of the required extensiona are not available. Patch does not include composer.lock updates.
50640-composer-lock.patch (2.5 KB) - added by ayeshrajans 9 days ago.
Same patch as before, but includes lock file updates too. Lock file is generated from composer update --lock on Composer 1.10.15. It slightly pollutes the diff because a dependency adds a new funding section, but the hashes should match for anyone running same latest Composer 1 version.

Download all attachments as: .zip

Change History (30)

#2 @johnbillion
4 months ago

  • Keywords close added

The problem with skipping tests when the platform requirements aren't met is that it removes assurance that the tests are actually running. If the test infrastructure changes in the future and a platform requirement such as gd gets accidentally gets removed, the tests should fail.

I recommend wontfix as this reduces the reliability of the tests.

#3 @ayeshrajans
4 months ago

With due respect, I would like to appeal for the ticket closure.

WordPress already has several tests being skipped due to extension availability:

  • mbstring
  • xsl
  • fileinfo
  • openssl
  • zip

Further, we already have Tests_Image_Meta class that skips the whole test class if gd extension is not available, which is precisely what we do in this test too. I agree about your point that this reduces test reliability, but failing a test does not improve the system we are testing anyway. Travis CI, for example, will have the gd extension in official PHP builds (all versions WordPress officially supports have Travis tests passing).

So on Travis, we will be running the exact same amount of assertions. We just avoid marking a test as "failure", which should have been a "skipped" test in a system that does not have necessary extensions to start the test in the first place.

#4 @ayeshrajans
4 months ago

If the test infrastructure changes in the future and a platform requirement such as gd gets accidentally gets removed, the tests should fail.

If we were to make sure the test platform meets all necessary requirements, I think it should be better done at composer.json file require-dev section (for example with ext-dev: *, ext-mbstring: *).

#5 @SergeyBiryukov
4 months ago

For reference, these tests were previously changed from skipping to failures in [40532] / #40533, the patch would revert that decision.

#6 @desrosj
4 months ago

  • Keywords php8 added

#7 @jrf
3 months ago

  • Keywords close removed

I concur with @ayeshrajans here. Tests which depend on certain environment elements should be skipped if the environment does not comply, not failed.

IF the tests have hard requirements for the environment and we want builds to fail on a mismatched environment, a check for those requirements should be added to the test bootstrap file instead with a helpful error message followed by a die(1) to make the test run fail.

(in addition to the Composer ext requirements as suggested by @ayeshrajans)

#8 @johnbillion
3 months ago

I'm sorry but tests which are skipped when the environment doesn't meet the requirements are inherently unreliable. This is why the skipping was removed originally.

IMO there should be a hard requirement on GD, Imagick, and any other extensions that are used within the tests, especially now we've got a local development environment available that ships in the repo. It should be able to provide the required extensions and always pass the corresponding tests.

The @requires annotation in PHPUnit should be used for such tests, I'm not sure why it's not used in core anywhere.

#9 @jrf
3 months ago

@johnbillion That kind of sounds like we completely agree ?

Or is there a misconception here on what the @requires annotations does ? (skip a test & mark it as skipped when the requirement is not met)

#10 @SergeyBiryukov
6 weeks ago

  • Milestone changed from Awaiting Review to 5.6

There is some overlap with #50639 here, moving to the milestone to address one way or another.

Looking at the current test failures, quite a few are caused by missing 'gd' extension in the WP Docker image. That includes tests explicitly checking for imagejpeg() and failing with "jpeg support unavailable", as well as tests implicitly relying on GD availability without specifically checking for it, causing more obscure failures.

The Docker image should probably be updated to provide the same list of extensions for PHP 8 that we currently have for PHP 7.4 and older: gd, opcache, mysqli, zip, exif, intl, mbstring, xml, xsl. If anyone is able to help with that and open a PR, please do :)

Still, explicitly requiring imagejpeg() and related functions as outlined in comment:3:ticket:50639 would be better than the current obscure failures. So consistently requiring GD functions where needed seems like the next step here. Whether that means failing or skipping the test, can be determined later.

Last edited 5 weeks ago by SergeyBiryukov (previous) (diff)

#11 @SergeyBiryukov
6 weeks ago

In 49009:

Tests: Skip some image tests if neither GD nor Imagick image editor engines are supported on the system.

The explicit message brings some consistency with other image editor tests, specifically the ones using the WP_Image_UnitTestCase class.

Previously, the tests were marked as "risky" in that scenario, due to performing no assertions.

See #50639, #50640.

#12 @SergeyBiryukov
6 weeks ago

In 49010:

Tests: Consistently require imagejpeg() function in image_make_intermediate_size() tests.

This outputs a proper message if the requirement is not met, instead of an obscure PHP error further in the test.

See #50639, #50640.

#13 @SergeyBiryukov
6 weeks ago

In 49014:

Tests: Optimize some image tests to avoid checking for image editor engines availability twice.

Follow-up to [49009].

See #50639, #50640.

#14 @SergeyBiryukov
6 weeks ago

In 49018:

Tests: Correctly unset non-supported image editor engines in some image tests.

Follow-up to [49009], [49014].

See #50639, #50640.

#15 @SergeyBiryukov
6 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#16 @SergeyBiryukov
6 weeks ago

In 49024:

Tests: Convert the checks for imagejpeg() function availability to use the @requires annotation.

This better utilizes the PHPUnit native functionality.

Props ayeshrajans, jrf, johnbillion.
Fixes #50639. See #50640.

#17 follow-up: @SergeyBiryukov
6 weeks ago

Keeping this open for now to address two things:

  • Update the WP Docker image do include the GD extension for PHP 8, per comment:10.
  • Add the GD requirement to the test bootstrap file and composer.json, per comment:7.
Last edited 6 weeks ago by SergeyBiryukov (previous) (diff)

#18 @SergeyBiryukov
6 weeks ago

In 49025:

Tests: Convert a few more function_exists() and extension_loaded() checks to @requires annotations.

This better utilizes the PHPUnit native functionality.

Follow-up to [49024].

See #50639, #50640.

#19 @SergeyBiryukov
5 weeks ago

In 49045:

Tests: Require imagejpeg() function in some more media tests.

This outputs a proper message if the requirement is not met, instead of an obscure PHP error further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025].

See #50639, #50640.

#20 @SergeyBiryukov
5 weeks ago

In 49047:

Tests: Require imagejpeg() function in some REST API attachments controller tests.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045].

See #50639, #50640.

#21 follow-up: @mikeschroder
5 weeks ago

Just in case it is helpful, this can be used to check if core supports JPEG with any of the editors present:
wp_image_editor_supports( array( 'mime_type' => 'image/jpeg' ) )

I recognize that it may not fit some of the requirements here.

I'll also echo that I agree that verbose failures should be present if the requirements are not met for tests.

#22 in reply to: ↑ 21 @SergeyBiryukov
5 weeks ago

Replying to mikeschroder:

Just in case it is helpful, this can be used to check if core supports JPEG with any of the editors present:
wp_image_editor_supports( array( 'mime_type' => 'image/jpeg' ) )

Thanks for sharing that! It is indeed helpful and could be used in some of tests above that don't explicitly rely on GD functions, but rather on JPEG support being present in general.

That said, since GD should already be a hard requirement for the test suite per the discussion above, the end result would probably be the same as with @requires function imagejpeg.

I don't have a strong preference here at the moment, but will consider switching to wp_image_editor_supports() where appropriate, for clarity.

#23 @SergeyBiryukov
5 weeks ago

In 49050:

Tests: Require imagejpeg() function in one more media test.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

This test relies on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045].

See #50639, #50640.

#24 @SergeyBiryukov
5 weeks ago

In 49052:

Tests: Require imagejpeg() function in WP_Widget_Media_Image::render_media() test.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

This test relies on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045], [49050].

See #50639, #50640.

#25 in reply to: ↑ 17 @desrosj
5 weeks ago

Replying to SergeyBiryukov:

Keeping this open for now to address two things:

  • Update the WP Docker image do include the GD extension for PHP 8, per comment:10.

https://github.com/WordPress/wpdev-docker-images/pull/36 is now open to bring the PHP 8 Docker container to a state to the ones for other PHP versions with two exceptions: Xdebug and Memcached are still not bundled. These extensions need to be fixed upstream before fully supporting PHP 8.

#26 @SergeyBiryukov
5 weeks ago

In 49069:

Tests: Require imagejpeg() function in Ajax media editing tests.

This outputs a proper message if the requirement is not met, instead of an obscure failure further in the test.

These tests rely on multiple resized copies of a test JPEG image being generated and available.

Follow-up to [49010], [49024], [49025], [49045], [49050], [49052].

See #50639, #50640.

#27 @SergeyBiryukov
11 days ago

  • Type changed from enhancement to task (blessed)

One item left to address from comment:17:

  • Add the GD requirement to the test bootstrap file and composer.json, per comment:7.

@ayeshrajans
9 days ago

Patch to add the composer.json entry, and a circuit-breaker in bootstrap file. It has an array of requirements, and shows a if any of the required extensiona are not available. Patch does not include composer.lock updates.

@ayeshrajans
9 days ago

Same patch as before, but includes lock file updates too. Lock file is generated from composer update --lock on Composer 1.10.15. It slightly pollutes the diff because a dependency adds a new funding section, but the hashes should match for anyone running same latest Composer 1 version.

Note: See TracTickets for help on using tickets.