Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Task to convert modules/system/lib/Drupal/system/Tests/Datetime/DateTimePlusTest.php
to use phpunit.
Comment | File | Size | Author |
---|---|---|---|
#23 | system-datetime-phpunit-2003698-23.patch | 35.11 KB | ParisLiakos |
#23 | interdiff.txt | 522 bytes | ParisLiakos |
#21 | interdiff.txt | 4.95 KB | jhedstrom |
#21 | system-datetime-phpunit=2003698-21.patch | 35.1 KB | jhedstrom |
#15 | interdiff.txt | 13.17 KB | jhedstrom |
Comments
Comment #1
jhedstromVery straightforward.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthis is a perfect match for dataproviders!
we should convert this test to use them instead:)
Comment #4
jhedstromHere's an initial conversion to using dataProviders. The string and array tests were easy to merge, the remaining tests vary in structure enough that they may require a separate data provider method.
Comment #5
jhedstromBumping status for tests since first patch never ran.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedwe shouldnt use global classes, just prefix them with \ in namespaced code.
eg new \DateTimeZone
Great conversion to dataprovider for testDates.
we should convert the rest methods too though.
also this provider should be named providerTestDates so we know where its used
Comment #7
jhedstromStill work to be done, but I ran out of time converting more to dataproviders. Marking needs review to see what the testbot thinks.
Comment #8
jhedstromConverted one more to dataproviders, and addressed the concern re: global classes.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedgreat already started looking a lot better:)
i meant we dont need a use statement at all
Just when referring to it in your code prefix it.
we can remove this now:)
this doc line misses * in the start
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedComment #11
jhedstromFixed #9, moved one more to dataproviders. The remaining two tests that aren't using dataproviders will need to perhaps be split up, as they aren't easily converted to dataproviders as currently written.
Comment #12
jhedstromThis gets the last 2 tests to use dataproviders. However, even though there were 2 tests (
testDateTimestamp
andtestTimezoneConversion
), they were almost identical. In this patch those have been merged.Comment #13
jhedstromI should also mention that the number of arguments being passed via dataproviders seems extreme, but I'm not sure what else one would do here.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedAwesome, great job thanks a lot for your time!
some suggestions below:
some of them need visibility..also param docs
this doc line misses *
to make the parameters less:
maybe put those in an array $initial
and those in an array $transform
so your test method ends up like this:
Comment #15
jhedstromI've simplified the methods with the ridiculous amount of input parameters as suggested in #14. Also cleaned up missing '*' and missing visibility declarations, and added param docs.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedGreat, way better:)
Almost there. Just wondering
why not use directly the array?
Comment #17
jhedstromI started to use the array directly, but items like this started looking quite messy that way.
I could clean it up a bit by using
sprintf()
in combination with using the array directly, but I'm not sure that makes it more readable or not.Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, that should definitely be sprintf..concatenating em like that..i dont know how this got in originally:)
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedAlso you can definitely remove htose messages...PHPunit will display a message which is almost identical to that..
Comment #21
jhedstromI didn't remove any messages, as they all seemed to contain a bit more info than default phpunit would offer. This patch removes the extra vars added in #15, and converts messages to use
sprintf()
.Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedthis looks really great, thanks!
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commented@dawehner noticed that the @file docblock is wrong
Comment #24
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #25
alexpottCommitted 1cac317 and pushed to 8.x. Thanks!
Fixed a couple of nits during commit...
Extra space
Needs to be \\ to escape it
Comment #26
claudiu.cristeaMaybe related? #2036081: DateTimePlusTest are failing in phpunit
Comment #28
jhedstrom