Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
2.11 KB

This one is quite simple as well.

ParisLiakos’s picture

Status: Needs review » Needs work

PercentagesUnitTest should be PercentagesTest, no point having Unit in the name for PHPUnit tests.

Also:

$this->testCases = array(
      // 1/2 is 50%.
      '50' => array('total' => 2, 'current' => 1),
      // Though we should never encounter a case where the current set is set
      // 0, if we did, we should get 0%.
      '0' => array('total' => 3, 'current' => 0),
      // 1/3 is closer to 33% than to 34%.
      '33' => array('total' => 3, 'current' => 1),
      // 2/3 is closer to 67% than to 66%.
      '67' => array('total' => 3, 'current' => 2),
      // 1/199 should round up to 1%.
      '1' => array('total' => 199, 'current' => 1),
      // 198/199 should round down to 99%.
      '99' => array('total' => 199, 'current' => 198),
      // 199/200 would have rounded up to 100%, which would give the false
      // impression of being finished, so we add another digit and should get
      // 99.5%.
      '99.5' => array('total' => 200, 'current' => 199),
      // The same logic holds for 1/200: we should get 0.5%.
      '0.5' => array('total' => 200, 'current' => 1),
      // Numbers that come out evenly, such as 50/200, should be forced to have
      // extra digits for consistancy.
      '25.0' => array('total' => 200, 'current' => 50),
      // Regardless of number of digits we're using, 100% should always just be
      // 100%.
      '100' => array('total' => 200, 'current' => 200),
      // 1998/1999 should similarly round down to 99.9%.
      '99.9' => array('total' => 1999, 'current' => 1998),
      // 1999/2000 should add another digit and go to 99.95%.
      '99.95' => array('total' => 2000, 'current' => 1999),
      // 19999/20000 should add yet another digit and go to 99.995%.
      '99.995' => array('total' => 20000, 'current' => 19999),
      // The next five test cases simulate a batch with a single operation
      // ('total' equals 1) that takes several steps to complete. Within the
      // operation, we imagine that there are 501 items to process, and 100 are
      // completed during each step. The percentages we get back should be
      // rounded the usual way for the first few passes (i.e., 20%, 40%, etc.),
      // but for the last pass through, when 500 out of 501 items have been
      // processed, we do not want to round up to 100%, since that would
      // erroneously indicate that the processing is complete.
      '20' => array('total' => 1, 'current' => 100/501),
      '40' => array('total' => 1, 'current' => 200/501),
      '60' => array('total' => 1, 'current' => 300/501),
      '80' => array('total' => 1, 'current' => 400/501),
      '99.8' => array('total' => 1, 'current' => 500/501),
    );

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

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
12.45 KB

This patch uses dataProviders, and has an initial Batch component (just moved the _batch_api_percentage function for now).

jhedstrom’s picture

Oops, forgot to change test method name as per #2.

jhedstrom’s picture

Forgot to rename the file to match the new class name.

dawehner’s picture

FileSize
4.12 KB
13.03 KB

Just a couple of nitpicks which makes it easier to use storm (for example match classname + test classname)

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-2003736-6.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit

#6: drupal-2003736-6.patch queued for re-testing.

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Component/Batch/PercentagesTest.phpundefined
@@ -0,0 +1,100 @@
+  public function provider() {

maybe 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:)

ParisLiakos’s picture

but, 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

dawehner’s picture

FileSize
13 KB

I 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.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-2003736-10.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit

#11: drupal-2003736-10.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Core/Batch/PercentagesTest.phpundefined
@@ -0,0 +1,100 @@
+  public function formatData() {

should 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

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.41 KB
6.41 KB

besides that, as we already discussed in irc with @dawehner we should move the batch.storage inside the Drupal\Core\Batch namespace

Opened a new issue for that: #2018257: Move batch storage to Drupal\Core\Batch

dawehner’s picture

FileSize
12.57 KB

urgs

ParisLiakos’s picture

Status: Needs review » Needs work

#14 comment still applies

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
14.32 KB

That's easy.

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Core/Batch/PercentagesTest.phpundefined
@@ -0,0 +1,100 @@
+  public function testPercentages($total, $current, $expected_result) {
...
+  public function providerFormatData() {

no, i mean the provider method name..should be providerTestPercentages

dawehner’s picture

FileSize
873 bytes
12.58 KB

Let's skip the moving for now and just rename the provider method.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

great, thanks good to go

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 69f2cef and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.