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

[ayeshrajans]

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.

[ayeshrajans]

Tests: ​https://travis-ci.com/github/Ayesh/wordpress-develop/builds/175322068

[johnbillion]

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.

[ayeshrajans]

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.

[ayeshrajans]

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: *).

[SergeyBiryukov]

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

[jrf]

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)

[johnbillion]

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.

[jrf]

@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)

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 49014:

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

Follow-up to [49009].

See #50639, #50640.

[SergeyBiryukov]

In 49018:

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

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

See #50639, #50640.

[SergeyBiryukov]

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.

[SergeyBiryukov]

Keeping this open for now to address two things:

• Update the WP Docker image do include the GD extension, per comment:10.
• Add the GD requirement to the test bootstrap file and composer.json, per comment:7.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[kirasong]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[desrosj]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

One item left to address from comment:17:

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

[ayeshrajans]

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]

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.

[SergeyBiryukov]

In 49535:

Build/Test Tools: Check if all the required PHP extensions are loaded before running the test suite.

Add the GD extension as a hard requirement.

This improves the reliability of the test suite and ensures that if the test infrastructure changes in the future and a platform requirement such as GD accidentally gets removed, the tests fail with an appropriate error message.

Follow-up to [48592].

Props ayeshrajans, jrf, johnbillion.
Fixes #50640.

[desrosj]

I think I’d like to suggest discussing backporting these improvements, within reason.

With the local Docker environment now backported through 4.6, the extensions will be available in all of those branches. Having some of these improvements would ensure that they’re properly tested and help prevent regressions when backporting security fixes.

[SergeyBiryukov]

Replying to desrosj:

I think I’d like to suggest discussing backporting these improvements, within reason.

Sounds good to me :)

[jamescollins]

One side effect of [49535] is any developers that are using ​WordPress core's unit testing library to execute their own tests (ie unit test their own plugins) now need to have GD installed.

This is a new requirement because our plugins don't do anything image related, and we don't run core tests as part of our plugin tests.

Is it possible to have this new check filterable, or even disabled via a new constant? So that third parties not needing GD for their own WordPress test suites don't have to install GD as part of every CI build?

Thank you.

[SergeyBiryukov]

Replying to jamescollins:

Is it possible to have this new check filterable, or even disabled via a new constant? So that third parties not needing GD for their own WordPress test suites don't have to install GD as part of every CI build?

Thanks for bringing that up! I think the PHP check can depend on the existing WP_RUN_CORE_TESTS constant.

Not sure what to do with the Composer dependency though, should we remove that and only rely on the PHP check?

This allows other users of the WordPress unit test suite framework to run their own unit tests without needing the GD extension, which should only be a requirement if running core tests.

To skip the required PHP extension checks, set WP_RUN_CORE_TESTS to false.

Trac ticket: https://core.trac.wordpress.org/ticket/50640

Hi @om4james! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in ​this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the ​Automated Testing page in the handbook.

If you have not had a chance, please review the ​Contribute with Code page in the ​WordPress Core Handbook.

The ​Developer Hub also documents the various ​coding standards that are followed:

• ​PHP Coding Standards
• ​CSS Coding Standards
• ​HTML Coding Standards
• ​JavaScript Coding Standards
• ​Accessibility Coding Standards
• ​Inline Documentation Standards

Thank you,
The WordPress Project

Hi @om4james! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in ​this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the ​Automated Testing page in the handbook.

If you have not had a chance, please review the ​Contribute with Code page in the ​WordPress Core Handbook.

The ​Developer Hub also documents the various ​coding standards that are followed:

• ​PHP Coding Standards
• ​CSS Coding Standards
• ​HTML Coding Standards
• ​JavaScript Coding Standards
• ​Accessibility Coding Standards
• ​Inline Documentation Standards

Thank you,
The WordPress Project

[jamescollins]

Hi @SergeyBiryukov,

Replying to SergeyBiryukov:

Thanks for bringing that up! I think the PHP check can depend on the existing WP_RUN_CORE_TESTS constant.

That's a great suggestion I think, so I've gone ahead and submitted a patch/PR that implements that change.

Not sure what to do with the Composer dependency though, should we remove that and only rely on the PHP check?

That decision could go either way I think. When installing WordPress core via composer, we don't know if the user intends to run the core unit tests at that point.

Given that the GD dependency check runs during bootstrap by default, I vote to remove ext-gd from composer.json, and just rely on the bootstrap check. That way composer installs aren't restricted and continue to be flexible depending on the use case.

And then if a user really doesn't intend to run the core unit tests and they're encountering the GD check during bootstrap, they can set WP_RUN_CORE_TESTS to false and all will be well.

Let me know if you want me to update my patch/PR to remove the composer dependency check too.

[SergeyBiryukov]

In 49571:

Build/Test Tools: Only enforce PHP extension requirements when running core tests.

This allows other users of the WordPress unit test suite framework to run their own unit tests without needing the GD extension, which should only be a requirement if running core tests.

Follow-up to [49535].

Props jamescollins.
Fixes #50640.

Merged in [49571]

[desrosj]

Reading through this ticket, I mistakenly tagged as php8. Removing that keyword.