Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
21.79 KB

Very straightforward.

Status: Needs review » Needs work

The last submitted patch, system-datetime-phpunit-2003698-01.patch, failed testing.

ParisLiakos’s picture

this is a perfect match for dataproviders!
we should convert this test to use them instead:)

jhedstrom’s picture

Here'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.

jhedstrom’s picture

Status: Needs work » Needs review

Bumping status for tests since first patch never ran.

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,369 @@
+use DateTimeZone;

we shouldnt use global classes, just prefix them with \ in namespaced code.

eg new \DateTimeZone

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,369 @@
+  public function provider() {

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

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
33.81 KB
4.14 KB

Still work to be done, but I ran out of time converting more to dataproviders. Marking needs review to see what the testbot thinks.

jhedstrom’s picture

Converted one more to dataproviders, and addressed the concern re: global classes.

ParisLiakos’s picture

great already started looking a lot better:)

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,348 @@
+use \DateTimeZone;

i meant we dont need a use statement at all
Just when referring to it in your code prefix it.

// No use statement up here
class myClass {
  // do stuff
  $date = new \DateTimeZone();
  // keep doing stuff
}
+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Set up required modules.
+   */
+  public static $modules = array();

we can remove this now:)

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,348 @@
+   *   - 'format' - Date format for DateTimePlus.
+       - 'format_date' - Date format for use in $date->format() method.
+   *   - 'expected' - The expected return from DateTimePlus.

this doc line misses * in the start

ParisLiakos’s picture

Status: Needs review » Needs work
jhedstrom’s picture

Fixed #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.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
33.42 KB
13.05 KB

This gets the last 2 tests to use dataproviders. However, even though there were 2 tests (testDateTimestamp and testTimezoneConversion), they were almost identical. In this patch those have been merged.

jhedstrom’s picture

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

ParisLiakos’s picture

Awesome, great job thanks a lot for your time!
some suggestions below:

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,341 @@
+  function testDateTimestamp($input, $timezone_initial, $format_initial, $expected_initial_date, $expected_initial_timezone, $expected_initial_offset, $timezone_transform, $format_transform, $expected_transform_date, $expected_transform_timezone, $expected_transform_offset) {
...
+  function testDateFormat($input, $timezone, $format, $format_date, $expected) {
...
+  function testInvalidDates($input, $timezone, $format, $message) {
...
+  public function testDateTimezone($date_input, $timezone_input, $expected_timezone, $message) {

some of them need visibility..also param docs

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,341 @@
+   *   - 'expected' - Expected return from DateTimePlus::getTimezone()::getName().
+   '   - 'message' - Message to display on test failure.
+   *

this doc line misses *

to make the parameters less:

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,341 @@
+        'timezone_initial' => 'UTC',
+        'format_initial' => 'c',
+        'expected_initial_date' => '1970-01-01T00:00:00+00:00',
+        'expected_initial_timezone' => 'UTC',
+        'expected_initial_offset' => 0,

maybe put those in an array $initial

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,341 @@
+        'timezone_transform' => 'America/Los_Angeles',
+        'format_transform' => 'c',
+        'expected_transform_date' => '1969-12-31T16:00:00-08:00',
+        'expected_transform_timezone' => 'America/Los_Angeles',
+        'expected_transform_offset' => '-28800',

and those in an array $transform

so your test method ends up like this:

  public function testDateTimestamp($input, array $initial, array $transform) {
    // test here
  }
jhedstrom’s picture

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

ParisLiakos’s picture

Great, way better:)
Almost there. Just wondering

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,425 @@
+    $timezone_initial            = $initial['timezone'];
+    $format_initial              = $initial['format'];
+    $expected_initial_date       = $initial['expected_date'];
+    $expected_initial_timezone   = $initial['expected_timezone'];
+    $expected_initial_offset     = $initial['expected_offset'];
+    $timezone_transform          = $transform['timezone'];
+    $format_transform            = $transform['format'];
+    $expected_transform_date     = $transform['expected_date'];
+    $expected_transform_timezone = $transform['expected_timezone'];
+    $expected_transform_offset   = $transform['expected_offset'];

why not use directly the array?

jhedstrom’s picture

I started to use the array directly, but items like this started looking quite messy that way.

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,425 @@
+    $value = $date->format($format_initial);
+    $this->assertEquals($expected_initial_date, $value, "Test new DateTimePlus($input, $timezone_initial): should be $expected_initial_date, found $value.");

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.

ParisLiakos’s picture

yeah, that should definitely be sprintf..concatenating em like that..i dont know how this got in originally:)

ParisLiakos’s picture

Also you can definitely remove htose messages...PHPunit will display a message which is almost identical to that..

Status: Needs review » Needs work

The last submitted patch, system-datetime-phpunit-2003698-15.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
35.1 KB
4.95 KB

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

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

this looks really great, thanks!

ParisLiakos’s picture

@dawehner noticed that the @file docblock is wrong

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1cac317 and pushed to 8.x. Thanks!

Fixed a couple of nits during commit...

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,413 @@
+   *     DateTimePlus: :setTimezone().

Extra space

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.phpundefined
@@ -0,0 +1,413 @@
+    $this->assertEquals($transform['expected_date'], $value, sprintf("Test \$date->setTimezone(new \DateTimeZone(%s)): should be %s, found %s.", $transform['timezone'], $transform['expected_date'], $value));

Needs to be \\ to escape it

claudiu.cristea’s picture

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

jhedstrom’s picture