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.
Task to convert modules/system/lib/Drupal/system/Tests/Common
to phpunit where possible.
Common/JsonUnitTest.php
Common/ValidUrlUnitTest.php
Common/ParseInfoFileUnitTest.php
Common/HtmlIdentifierUnitTest.php
Common/AutocompleteTagsUnitTest.php
Common/AttributesUnitTest.php
Common/SizeUnitTest.php
Common/TableSortExtenderUnitTest.php
Common/CascadingStylesheetsUnitTest.php
Common/DiffArrayUnitTest.php
Common/ValidNumberStepUnitTest.php
Common/ColorTest.php
Comment | File | Size | Author |
---|---|---|---|
#17 | common-2003568-17.patch | 23.8 KB | dawehner |
#17 | interdiff.txt | 6.21 KB | dawehner |
#13 | drupal-2003568-13.patch | 23.2 KB | dawehner |
#13 | interdiff.txt | 10.79 KB | dawehner |
#11 | drupal-2003568-11.patch | 22.85 KB | dawehner |
Comments
Comment #1
jhedstromThis patch is quite involved compared to some of the others. A lot of these tests required constants and functions defined in
includes/bootstrap.inc
, so that got added totests/bootstrap.php
--not sure if there's currently a way around that.Comment #2
jhedstromI should have mentioned,
Common/CascadingStylesheetsUnitTest.php
could not be easily converted since it relies on comparing CSS files contents loaded from the file system, with those fetched from$GLOBALS['base_url']
via HTTP.Comment #4
Berdir#1: system-common-phpunit-2002568-01.patch queued for re-testing.
Comment #6
Berdir#1: system-common-phpunit-2002568-01.patch queued for re-testing.
Comment #8
BerdirThe test methods match the old function names. they should probably be renamed. Technically not strictly necessary but keeping them would be kinda weird I think.
Wondering if I haven't seen this already somewhere, didn't that get in yesterday or so?
Not sure if we even still need this, given that we converted everything to yml, including the name of this test :p
And including bootstrap.inc/common.inc is a no-go I think, I'd rather convert those tests to DUBT or rewrite the code to use classes and static methods like we did for other examples.
Comment #9
dawehnerAnother classical example that you should never do too much in one issue.
I removed the conversions which didn't had conversions in bootstrap.inc and moved the explode/implode to a tags class.
Comment #10
jhedstromOn a few of the other component conversions, the
@deprecated
tag has been added to the wrapper function. I think we should do that in this case as well.Also, great idea on reducing the scope of this issue.
Comment #11
dawehnerGood idea, let's do it here.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedunneeded changes
this should use a dataprovider
needs visibility
lets prefix this with provider:)
those could be merged into the provider
provider prefix instead too
same as well
and all test below in same pattern
Comment #13
dawehnerGreat review! Fixed a couple of more points like renaming the test classes etc.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commented#13: drupal-2003568-13.patch queued for re-testing.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedyou missed an 'r'
also, i am sorry i failed to mention above..we tend to add the Test..
so providerTestAttributeData
providerTestValideAbsolute
likewise
providerTestInvalidAbsolute
we should move this foreach to the provider instead.
should be protected right?
Comment #17
dawehnerFixed all the points
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedcool, its ready to go, thanks!
Comment #19
alexpottCommitted 573068f and pushed to 8.x. Thanks!
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedseems this test fails on 5.5 #2068593: JsonTest fails with pecl-json
Comment #22
jhedstrom