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.There are a few minor issues with tests in this project:
a) from andypost: setup should return void and modules property protected in tests/src/Functional/LangCodeTest
b) from megachriz: In tests/src/Unit/Porter2Pecl1.php the following line:
$this->assertTrue(TRUE, 'No PECL stem library found, Aborting test.');
It would be better to use a markTestSkipped() call.
https://phpunit.readthedocs.io/en/9.0/incomplete-and-skipped-tests.html#...
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | interdiff_4_18.patch | 3.42 KB | sanjayk |
| #18 | 3130380-18.patch | 4.27 KB | sanjayk |
| #12 | 3130380-7.patch | 3.99 KB | sanjayk |
| #4 | minor_test_fix_ups-3130380-4.patch | 3.35 KB | anisha.challa |
Comments
Comment #2
anisha.challa CreditAttribution: anisha.challa as a volunteer and at TA Digital commentedComment #3
dastan56 CreditAttribution: dastan56 as a volunteer commentedComment #4
anisha.challa CreditAttribution: anisha.challa as a volunteer and at TA Digital commentedHi,
I have added the patch for review. I haven't replaced the public static of modules property to protected as it is a static property.
Please review the patch.
Comment #5
jhodgdonThanks for the patch!
I think the calls to markTestSkipped are incorrect:
https://phpunit.readthedocs.io/en/9.0/incomplete-and-skipped-tests.html#...
It looks like it wants just one parameter (message string).
Also regarding $modules, the base class this derives from declares it as
so we should do the same thing.
Comment #6
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedComment #7
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@jhodgdon Made changes as suggested please review
Comment #8
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedComment #9
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedFixed the issue in patch
Comment #10
jhodgdonThis patch in #9 is not correct -- it is not calling markTestSkipped() any more, and it should be. Neither is the patch in #7.
We need to go back to the patch in #4 and fix the problems noted in #5.
The first fix about the declaration of $modules is correct, but what needs to happen to markTestSkipped() is you need to look up the function signature of that in the phpUnit documentation (try a Google or other web search to find it), and make the calls be correct. Namely, this function has only 1 parameter, but the calls in the patch in #4 passed in 2 parameters.
Comment #11
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedComment #12
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedSorry I missed it. Now I fixed.
Comment #13
jhodgdonLooks good, thanks!
After applying the patch, I looked for $modules, setUp() and the aborted test string in the tests, and it all looks right, except that I found one more place that needs a similar fix:
Comment #14
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@jhodgdon fixed #13 in new patch please review.
Comment #15
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedPlease ignore #14
Comment #16
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@jhodgdon I have already fixed same changes in two "protected static $modules". You can check in patch #12 and it was in files tests/src/Unit/PorterPeclBase.php and tests/src/Functional/LangCodeTest.php.
Comment #17
jhodgdonI think you misunderstood #13. I meant to say that the setUp() function is not having the :void typehint in PorterPeclBase. You are correct that the $modules variable is correct.
Comment #18
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@jhodgdon thanks, now fixed #17 please review.
Comment #19
jhodgdonIt really helps if you make an interdiff file whenever you upload a new patch on an issue, thanks! It makes it much easier for someone to see whether you have addressed feedback.
Comment #20
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@jhodgdon Sorry, I was forgot to upload interdiff file. Test cases already passed please ignore current one #20.
Comment #21
jhodgdonThanks! The patch looks good now, and I've committed it.