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 Batch/PercentagesUnitTest.php
to phpunit.
Comment | File | Size | Author |
---|---|---|---|
#20 | batch-2003736-20.patch | 12.58 KB | dawehner |
#20 | interdiff.txt | 873 bytes | dawehner |
#18 | drupal-2003736-18.patch | 14.32 KB | dawehner |
#18 | interdiff.txt | 1.47 KB | dawehner |
#16 | drupal-2003736-15.patch | 12.57 KB | dawehner |
Comments
Comment #1
jhedstromThis one is quite simple as well.
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedPercentagesUnitTest
should bePercentagesTest
, no point having Unit in the name for PHPUnit tests.Also:
This should be a dataprovider
require_once DRUPAL_ROOT . '/core/includes/batch.inc';
This is kinda of showstopper i think:(
not sure if we plan to convert batch to a service or something
Comment #3
jhedstromThis patch uses dataProviders, and has an initial Batch component (just moved the _batch_api_percentage function for now).
Comment #4
jhedstromOops, forgot to change test method name as per #2.
Comment #5
jhedstromForgot to rename the file to match the new class name.
Comment #6
dawehnerJust a couple of nitpicks which makes it easier to use storm (for example match classname + test classname)
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commented#6: drupal-2003736-6.patch queued for re-testing.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedmaybe this name is too general and will bring troubles if we decide to move more tests in this class..behind this i believe batch have a potential as a component:)
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedbut, after seeing that we have batch.storage service i am not quite sure we want to go towards this way...unless we rename the component to Percentage component..because in the end we might end up with something very confusing
Comment #11
dawehnerI think as the batch is entirely part of core not just a component this helper method should also just be in core. I don't think it makes sense to move this methods to the batch storage as it's an orthogonal part of the code.
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commented#11: drupal-2003736-10.patch queued for re-testing.
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedshould be called providerTestPercentages
besides that, as we already discussed in irc with @dawehner we should move the batch.storage inside the Drupal\Core\Batch namespace
Comment #15
dawehnerOpened a new issue for that: #2018257: Move batch storage to Drupal\Core\Batch
Comment #16
dawehnerurgs
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commented#14 comment still applies
Comment #18
dawehnerThat's easy.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedno, i mean the provider method name..should be providerTestPercentages
Comment #20
dawehnerLet's skip the moving for now and just rename the provider method.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedgreat, thanks good to go
Comment #22
alexpottCommitted 69f2cef and pushed to 8.x. Thanks!