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.
child of #1938068: Convert UnitTestBase to PHPUnit
There are 12 files with UnitTestBase classes:
core/modules/system/lib/Drupal/system/Tests/Common/ColorTest.php
core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ValidNumberStepUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ParseInfoFileUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/HtmlIdentifierUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/SizeUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/JsonUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/DiffArrayUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/AutocompleteTagsUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/TableSortExtenderUnitTest.php
core/modules/system/lib/Drupal/system/Tests/Common/ValidUrlUnitTest.php
Maybe to many to do in one Issue?
Comment | File | Size | Author |
---|---|---|---|
#28 | drupal-color_test_phpunit-1938184-28.patch | 9.21 KB | ParisLiakos |
#28 | interdiff.txt | 1.8 KB | ParisLiakos |
#24 | drupal-color_test_phpunit-1938184-23.patch | 8.84 KB | ParisLiakos |
#12 | drupal-ColorTest-1938184-12.patch | 4.53 KB | clemens.tolboom |
#8 | drupal_1938184_03.patch | 1.09 KB | Xano |
Comments
Comment #1
franskuipers CreditAttribution: franskuipers commentedFirst patch for one test in system/lib/Drupal/system/Tests/Common directory: ColorTest.php
My remarks/questions:
Comment #2
franskuipers CreditAttribution: franskuipers commentedComment #3
XanoI did a manual review. The patch looks good.
Yes, these two are identical.
What was your reason for doing this?
Comment #4
franskuipers CreditAttribution: franskuipers commentedReason for bootstrap.inc
PHP Fatal error: Call to undefined function Drupal\Core\Utility\drupal_strlen() in /home/frans/workspace/drupal8/core/lib/Drupal/Core/Utility/Color.php on line 31
Reason for unicode.inc:
Comment #5
XanoRe-roll using DrupalUnitTestBase so we do not need workarounds.
Comment #6
clemens.tolboomIt's hard to see the changes in both patches :(
@Xano you should use "git diff -M --cached 8.x" ... or cf #1116900-4: git format-patch displays file moves as whole verbose diffs somehow.
See #1935908: Convert Graph tests to phpunit where the patch has a move and is still nice diffed.
Should we really move back to DrupalTestBase?
Comment #7
XanoYes, as this is a dependency injection problem. See New, separate, extended DrupalUnitTestBase for unit-testing functionality whose dependencies can't be injected.
Comment #8
Xano@clemens.tolboom: Thanks for explaining git diff -M!
The attached patch simply converts the test case to DrupalUnitTestBase, and it includes @franskuipers' original code comment updates.
Comment #9
franskuipers CreditAttribution: franskuipers commented#1: drupal-ColorTest-1938184-1.patch queued for re-testing.
Comment #10
BerdirThis is not correct. If it becomes a DUBT test, then it can not be moved to core/tests. Currently, only PhpUnit tests can be there.
You can see that this test class is no longer found in the output, there is a ColorTest but that's from color.module.
Comment #11
XanoAlso see #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode.
Comment #12
clemens.tolboomThe mistake we made is to convert from
class ColorTest extends UnitTestBase {
to PHPUnit just because it was named UnitTestBase. That did not imply it is a standalone Unit test.The effort @franskuipers did by adding bootstrap + unicode was breaking the unit test spirit.
The effort @Xano did was solving the includes but making it further down from unit test.
So the problem lies in fixing core/tests/bootstrap.php by adding the quirk like
Attached patch does the above but we need to discuss
drupal_strlen
for sure.After applying the patch either of the below could work. I have no clue how to apply the reverse of
core$ git diff 8.x -M > ~/Downloads/drupal-ColorTest-1938184-12.patch
to make the file move :(
Comment #13
clemens.tolboomComment #14
BerdirThe issue posted by Xano in #11 is actually the better approach, as it moves the functionality into a class that can also be used from a unit test.
Comment #15
clemens.tolboomArgh .. I assumed I put the moved lines @franskuipers did in #1 back into place which is not the case. I hope @franskuipers can redo the patch from #1 with the spirit from #12 :)
Comment #16
clemens.tolboomThe patch from #11 has no PHPUnit base class.
while
has. Or am I missing something vital now!?!
Comment #18
XanoThe current test uses UnitTestBase, which is the most 'unit' Simpletest test we have, so making the test extend DrupalUnitTestBase was a bad decision. The next step is to make it a PHPUnit test, which is impossible, because \Drupal\Core\Utility\Color depends on code that cannot be lazy-loaded yet (see #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode).
Postponing until that other issue is fixed. Then we can continue porting this test to PHPUnit.
Comment #19
franskuipers CreditAttribution: franskuipers commentedXanos diversion is about this code in #1. The lines
where brought in with the core patch in #1901670: Start using PHPUnit for unit tests.
I changed this by loading the includes, because I needed drupal_strlen() (in unicode.inc) and 'REQUEST_TIME' (in bootstrap.inc)
My reasoning for this temporary hack are:
There are open questions from #1:
I would like to help/move on with #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest but I am a bit lost now :)
Comment #20
XanoDrupalUnitTestBase is the official workaround for a case like this now. It prevents us from having to include everything explicitly, and from having to keep track of test-specific workarounds manually. You're absolutely right that it's not as performant as your solution, but it is much easier to maintain in the long run. Once the entire core codebase is either loaded by default or can be lazy-loaded, we can convert all DrupalUnitTestBase test cases to PHPUnit.
Comment #21
franskuipers CreditAttribution: franskuipers commented... deleted ....
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commented#1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode is rtbc..also retitling
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedas per #18
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedmuch better
Comment #25
clemens.tolboomCan you re-submit the patch with
git -M
as mentioned in #1938068: Convert UnitTestBase to PHPUnitComment #26
ParisLiakos CreditAttribution: ParisLiakos commentedi dont think it is that big patch, and the moved file is very different, so it helps to check whether the test was converted correctly
(usually git does this automatically for me, but this time the differences between files are many, there where no dataproviders before)
Comment #27
dawehnerI agree that git -M should be used, but if data providers are used (which seems way better in general) this kind of diff does may help less.
The tests are looking pretty great.
Data providers will probably take care about showing failing data?
Just a couple of nitpicks.
Needs a "\" as prefix.
Should this also have documentation?
Isn't it an array of arrays?
It seems to the case that "Color::rgbToHex" could need description how it works but this is for sure out of scope.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for review!
Yes, this how it looks in phpunit when something throws an unexpected exception:
fix rest points
about git -M..there is almost no similarity with those files, namespaces changed, functions changed, indentation changer
Comment #29
dawehnerThank you!
Comment #30
alexpottCommitted 7846ad6 and pushed to 8.x. Thanks!
Comment #31.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #32
jhedstrom