Test for ticket 39875 can throw PHP error

[schlessera]

The test that was written for #39875 can throw a PHP error under certain circumstances: Undefined index: sizes.

The problem arises when then wp_generate_attachment_metadata() call does not return the proper array data. As the preview path generation directly access a specific array index, this throws a PHP error.

[schlessera]

Check for valid $metadata array before accessing it

[netweb]

Related: #40537 PDF tests cannot be tested on Travis

[gitlost]

@schlessera there were similar checks in that patch originally but they were removed - see https://core.trac.wordpress.org/ticket/39875#comment:18 - and though I'd tend to add such checks I can see @joemcgill's point that it does bloat the tests...and as @netweb points out the PDF tests need fixing anyway...

[schlessera]

I agree that tests should be as lean as possible. But if you put these checks into a different test, you will still have this particular test throw a PHP error instead of failing. I think that tests should never throw errors, they should always only succeed or fail (or be skipped).

The problem with having the test be too complex in this case is an issue with the testability of the code, and not with the test itself.

[gitlost]

True enough. Under what circumstances is the PHP error happening?

[schlessera]

The test error happens when some or all of the dependencies (ghostscript, imagemagick, imagick) are not properly installed. It will result in an empty array being returned by wp_generate_attachment_metadata().

Alternatively to the asserts I added, we could add a check to see whether the proper dependencies are installed, and skip the tests with a corresponding notice if not.

[johnbillion]

Related: #40536, #40537.

Replying to schlessera:

Alternatively to the asserts I added, we could add a check to see whether the proper dependencies are installed, and skip the tests with a corresponding notice if not.

There's already a bunch of test skipping in place for other tests that depend on Imagick. The same pattern can be used.

In the long term, I think tests that require Imagick, GS, etc should fail (instead of be skipped) if the dependencies are not installed.