Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Doing some follow-through on #2728579: Explicitly skip @requires module in PHPUnit Kernel and Browser tests I discovered that ToolkitGdTest has a checkRequirements() method, which overrides \PHPUnit_Framework_TestCase::checkRequirements().
This is probably left over from the Simpletest days. It was converted from Simpletest with all system module tests in #2687897: Convert system module's kernel tests to NG
The problem is that it tries to signal its requirements by returning a value, even though PHPUnit ignores the returned value and expects an exception if the requirement isn't met.
Here's what it looks like:
protected function checkRequirements() {
// GD2 support is available.
if (!function_exists('imagegd2')) {
return [
'Image manipulations for the GD toolkit cannot run because the GD toolkit is not available.',
];
}
return parent::checkRequirements();
}
Proposed resolution
- Move the requirement for the
imagegd2()function to a@requiresannotation. - Remove the
checkRequirements()method.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | interdiff_20-24.txt | 593 bytes | tvb |
| #24 | 2861376-24.patch | 951 bytes | tvb |
| #21 | 2861376-20.patch | 1001 bytes | tvb |
| #7 | interdiff-2861376-4-7.txt | 380 bytes | chiranjeeb2410 |
| #7 | 2861376-7.patch | 887 bytes | chiranjeeb2410 |
Comments
Comment #2
Mile23Comment #3
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commentedWould be working on this one.
Comment #4
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@Mile23,
Uploading patch with required changes mentioned above.
Please review.
Comment #5
dawehnerAt last the official documentation don't use () on here: https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#i...
Comment #6
Mile23Comment #7
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@Mile23,
Did the required changes. Please review.
Comment #8
Mile23checkRequirements() imposes a requirement on the test class.
So the annotation should be part of the class docblock.
Check out the docs @dawehner linked to: https://phpunit.de/manual/current/en/incomplete-and-skipped-tests.html#i...
Comment #9
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@Mile23,
Fixed according to previous suggestion. Uploading fresh patch with interdiff.
Please review.
Comment #10
Mile23Since this is a testing bug it should be filed against 8.3.x.
The patch applies to both 8.3.x and 8.4.x.
LGTM, re-running the tests for both branches.
Comment #11
mondrakeShouldn't the @requires be in the class docblock? It's on a property right now AFAICS.
Comment #13
Mile23You're right. I missed that it was on the property and not the class docblock before.
Comment #14
Mile23Comment #17
vacho CreditAttribution: vacho at Skilld commentedComment #18
vacho CreditAttribution: vacho at Skilld commentedPatch updated.
Comment #19
mr.baileysSetting to Needs Review since there is a patch available for review.
I think we should check for the presence of the GD extension, rather than one function exposed by that extension, similar to ImageTest and ImageFormatterTest. So:
Comment #21
tvb CreditAttribution: tvb commented2861376-18.patch applies cleanly to 8.9.x-dev.
Rerolled the patch with the change proposed in #19.
Comment #23
mr.baileys#11:
The @requires-directive still needs to move to the class docblock.
Comment #24
tvb CreditAttribution: tvb commentedComment #25
quietone CreditAttribution: quietone as a volunteer commentedThis has chiranjeeb2410 assigned and that person hasn't worked on this issue for 3 years. I think it is safe to un-assign.
Comment #29
neclimdulApplied cleanly to both 10.0 and 9.4 and worked as expected.
Comment #30
alexpottBackported to 9.3.x as this is a test-only change.