Refactor KernelTests/Core/Common/SizeTest

Unified the two tests into a single test with a data provider and (hopefully) modernised it to current standards.

Thanks for your tests ! I would go even beyond on modernisation & best practices to avoid any business logic or any if conditions.

I would probably end up writing separate tests for the following if case

For non-rounded values, cross-test that Bytes::toInt() returns the original input.

It would be by fare more elegant & easier to read if you provide 2 dataProviders & split the tests on 2 separated logics.

Here is my suggestion

• testCommonFormatSize with a provider providerTestCommonFormatSize
• testCommonFormatSizeCrossTestToInt with a provider providerTestCommonFormatSizeToInt

What do you think ?

Here are my suggestions, what do you think about it ?
By decoupling, we can add a proper @covers and we simplify the test readability.

PS: We could maybe rename the testCommonFormatSizeCrossTestToInt to something better.

Oppsy I made a typo on my copy/past provider of tests ... sorry

+++ b/core/tests/Drupal/KernelTests/Core/Common/SizeTest.php
@@ -12,62 +12,66 @@
+  public function testCommonFormatSizeCrossTestToInt($expected, $size) {
+    $this->assertEquals($expected, Bytes::toInt($size));

This doesn't actually cross-test; it should call format_size($expected) and then check that Bytes::toInt() returns the original value.

This is the idea @longwave to rename this. UnitTest should not do some cross-test it's not a best practice. Otherwise if the first format_size fails it will obviously fail the "cross-test" which is not very useful. We can assert it works by himself just by using the provider.

If we want to assert Bytes::toInt we can do it in this test but we have to rename it, indeed as I suggest it in #8.

What do you think ? Let's discuss about it !

Here are the changes according #8, #11 & #12.

There is already a standalone test for Bytes::toInt() in \Drupal\Tests\Component\Utility\BytesTest, so I don't think we need another unit test of that method here?

The point of the cross-test seems to be ensuring that Bytes::toInt() can correctly reverse whatever format_size() produces (for non-rounded values at least), not sure whether that is actually worthwhile though.

Your are perfectly right, I didn't see the existing tests before !
Then we should remove the tests I added, my bad.

I don't think we have to cross-test this, I mean all the necessary assertions are already done by the existing \Drupal\Tests\Component\Utility\BytesTest::testToInt.

Let's remove my unnecessary testBytesToInt & providerTestBytesToInt.

Here are the changes according #2 and #13.

I remove testBytesToInt & providerTestBytesToInt. I added $exact as the default parameter.

Now it looks like this:

public function testCommonFormatSize($expected, $input, $exact = TRUE) {
  $size = format_size($input, NULL);
  $this->assertEquals($expected, $size);

  // For non-rounded values, cross-test that Bytes::toInt() returns the
  // original input.
  if ($exact) {
    $this->assertEquals($input, Bytes::toInt($size));
  }
}

public function providerTestCommonFormatSize() {
  $kb = Bytes::KILOBYTE;
  return [
    ['1 byte', 1],
    ['2 bytes', 2],
    ['1 KB', $kb],
    ['1 MB', pow($kb, 2)],
    ['1 GB', pow($kb, 3)],
    ['1 TB', pow($kb, 4)],
    ['1 PB', pow($kb, 5)],
    ['1 EB', pow($kb, 6)],
    ['1 ZB', pow($kb, 7)],
    ['1 YB', pow($kb, 8)],
    'Rounded to 1 MB - not 1000 or 1024 kilobyte' => ['1 MB', ($kb * $kb) - 1, FALSE],
    'Decimal Megabytes' => ['3.46 MB', 3623651, FALSE],
    'Decimal Petabytes' => ['59.72 PB', 67234178751368124, FALSE],
    'Decimal Yottabytes' => ['194.67 YB', 235346823821125814962843827, FALSE],
  ];
}

What do you think?

I think the following code is not readable.

'Rounded to 1 MB - not 1000 or 1024 kilobyte' => ['1 MB', ($kb * $kb) - 1, FALSE],
'Decimal Megabytes' => ['3.46 MB', 3623651, FALSE],
'Decimal Petabytes' => ['59.72 PB', 67234178751368124, FALSE],
'Decimal Yottabytes' => ['194.67 YB', 235346823821125814962843827, FALSE],

Better earlier version?

//'Rounded to 1 MB - not 1000 or 1024 kilobyte'
['1 MB', ($kb * $kb) - 1, FALSE],
//'Decimal Megabytes'
['3.46 MB', 3623651, FALSE],
//'Decimal Petabytes'
['59.72 PB', 67234178751368124, FALSE],
//'Decimal Yottabytes'
['194.67 YB', 235346823821125814962843827, FALSE],

Thanks for reroll & work on it.

For #16 the idea was to not do cross-test. So only remove the testBytesToInt & providerTestBytesToInt of #13 we don't want to mix 2 kind of assertion on the same test (plus BytesToInt is already covered).

About #17 I agree let's change it (again).

Don't forget to upload an interdiff on changes, would be easier to review :)

@Kevin Thanks for the previous tips!

I removed testBytesToInt & providerTestBytesToInt #15.
I corrected #17.

There is already a standalone test for Bytes::toInt() in \Drupal\Tests\Component\Utility\BytesTest, so I don't think we need another unit test of that method here?

The point of the cross-test seems to be ensuring that Bytes::toInt() can correctly reverse whatever format_size() produces (for non-rounded values at least), not sure whether that is actually worthwhile though.

The cross-test (with Bytes::toInt()) could look like following code, but it is not necessary. Let's not add this. Do you agree with me?

public function testCommonFormatSizeConversionTest($input) {
  $size = format_size($input, NULL);
  $this->assertEquals($input, Bytes::toInt($size));
}

public function providerTestCommonFormatSizeConversionTest() {
  $kb = Bytes::KILOBYTE;
  return [
    1,
    2,
    $kb,
    pow($kb, 2),
    pow($kb, 3),
    pow($kb, 4),
    pow($kb, 5),
    pow($kb, 6),
    pow($kb, 7),
    pow($kb, 8),
  ];
}

I agree with you as that's exactly what I try to explain (maybe badly ^^).

We don't need the test testCommonFormatSizeConversionTest for the following reasons:

The Bytes::toInt is already covered by \Drupal\Tests\Component\Utility\BytesTest::testToInt . And the result of format_size are covered by our new test testCommonFormatSize.

So both Bytes::toInt & format_size output/input are covered. if our tests on format_size fail then the output will be different as expected (aka not compliant with Bytes::toInt). We don't need to add a cross-test anywhere.

I can no longer RTBC this. Anyone else want to review / test / RTBC?

#19 looks great to me.

Much more readable and explicit - nice work - and once format_size becomes a class this can be easily converted to a unit test.

Committed and pushed f321b4aa69 to 8.7.x and 228c146440 to 8.6.x. Thanks!