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 core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php
to phpunit.
Comment | File | Size | Author |
---|---|---|---|
#42 | 2002116-42.patch | 16.3 KB | damiankloip |
#42 | interdiff-2002116-42.txt | 794 bytes | damiankloip |
#40 | 2002116-40.patch | 16.3 KB | damiankloip |
#38 | 2002116-38.patch | 16.3 KB | damiankloip |
#38 | interdiff-2002116-38.txt | 1.27 KB | damiankloip |
Comments
Comment #1
jhedstromAlso a pretty simple one. Will most likely need to remove the additional
Tests
directory from the new location for the file.Comment #2
dawehnerWe could throw out the "Unit" here, no need ... it will always be a unit test.
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthis is a no:) we should convert this to a service/component instead, and then convert the test
also +1 @#2
Comment #4
dawehnerThen let's do it!
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedneeds a docblock
lets remove the Unit from the name
should be
// First test...
Also should move to dataproviders:)
Comment #7
damiankloip CreditAttribution: damiankloip commentedLet's try and fix this, and incorporate that feedback.
I'm not sure about using a data provider here?
Comment #8
damiankloip CreditAttribution: damiankloip commentedYeah, of course it is! :) Let's rock this out with a data provider.
The name also needs to go back to UpdateCoreUnitTest because we get a class declaration error.
Comment #9
dawehnerAwesome!!
Let's add a todo here ...
What about a @inheritdoc here?
dataProvider all the things!
Needs docs, sorry.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentednot sure i get why
Comment #11
jhedstromre: #10 I don't quite understand why either. This patch removes 'Unit' from the name and still passes w/o an error.
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedalso should probably be named UpdateFetcherTest since thats the class it tests
Comment #14
damiankloip CreditAttribution: damiankloip commentedYeah, I think UpdateFetcherTest is a better name, Let's go with that.
RE #10, Did you see the test failures from the patch? These are the same as the ones in #7. So I'm guessing you just ran the single test from the CLI? Essentially that patch is the same as #7 which already failed for the same thing.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedcannot redeclare the class...i forgot simpletest just loads everything it can find during tests.
Anyway UpdateFetcherTest to the rescue;)
i guess we need to fix #9 and then rtbc this:)
Comment #16
jhedstromI think this addresses the remaining issues in #9.
Comment #18
damiankloip CreditAttribution: damiankloip commented#16: update-module-phpunit-2002116-16.patch queued for re-testing.
Comment #19
dawehnerJust a few nitpicks.
Let's add some docs.
and rename the provider to providerTestUpdateBuildFetchUrl
Comment #20
jhedstromThis should address #19.
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commented#20: update-module-phpunit-2002116-20.patch queued for re-testing.
Comment #23
dawehnerThis change is just out of scope, sorry.
I guess on the longrun the UpdateFetcher will do more then just building URL's?
let's typehing project with array
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedi fixed points above and also moved anything related with Guzzle into the service, since thats what you expect it to do anyway
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commented#24: update-module-phpunit-2002116-24.patch queued for re-testing.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedsigh, missed a use statement
Comment #29
dawehnerLet's use the ClientInterface
I guess we should have a unit test for this method as well.
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedI fixed the typehint.
i am not sure how to test this method, neither if i should..it has potential to cause failures under different enviroments, depending on the network settings
Comment #31
dawehnerIsn't the idea with guzzl that you can actually mock the http request?
Comment #32
dawehnerLet's get it in then.
Comment #33
catchConverting this to an injected class is good, but making it a service seems a bit like overkill to me - it's really not intended to be used outside of update module.
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedwell, the BookManager wont be used out of book module as well, but its a service since somehow we need to manage its dependencies. Same here.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commented#1987888: Convert update_manual_status() to a new style controller does the same btw
Comment #36
dawehnerWell you can always just define the depenencies in the controller you need for the custom class and inject it then.
Comment #37
Mile23Needs a re-roll.
Comment #38
damiankloip CreditAttribution: damiankloip commentedrerolled (quickly and it's early) and made UpdateFetcher not a service.
Comment #40
damiankloip CreditAttribution: damiankloip commentedMeh
Comment #42
damiankloip CreditAttribution: damiankloip commentedConstant needs to switch to \Drupal.
Comment #43
dawehnerIs there a follow up to remove this constant in update module itself?
Comment #44
webchickCommitted and pushed to 8.x. Thanks!
Comment #46
jhedstrom