Problem/Motivation

In the process of debugging a failing test in the DateTime Range module (issue forthcoming), I observed that the test loops over several edgecase timezones, but does not report which timezone is being tested. It would have been really helpful to know (for example) if this particular fail is always related to the Pacific/Tongatapu timezone each of the times I've seen it, or if was related only to (e.g.) the request time, with the timezone different between the fails I've seen.

Proposed resolution

Add the timezone to assertion messages and entity labels for relevant test code that loops over timezones.

Here's one line I added (for example):

       $edit = array(
+        'name[0][value]' => $timezone . ' ' . $this->randomMachineName(),
         "{$field_name}[0][value][date]" => $start_date->format($date_format),
         "{$field_name}[0][end_value][date]" => $start_date->format($date_format),

For the assertion messages, we could either add the timezone manually to relevant assertions, or do something fun with storing the current timezone on the class and overriding TestBase::assert() in the datetime test base class. The first option would be easier with a bit of refactoring since a number of methods seem to repeat similar test operations, but that would be a lot more work.

Remaining tasks

TBD

CommentFileSizeAuthor
#14 interdiff-12-14.txt2.1 KBmpdonadio
#14 2829845-14.patch13.07 KBmpdonadio
#12 2829845-12.patch13.07 KBjofitz
#5 2829845-05.patch11.31 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

For extra debugging glory we could also record the timestamp.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Active » Needs review
FileSize
2829845-05.patch11.31 KB

First pass at adding an assertion that we set the TZ properly when we are looping, and to add extra debug to the formatter assertion messages. Think I got them all in the field tests; may also want this in the views tests where we do the TZ loop.

Adding the actual timestamp may be beneficial, but it may cause more harm than good because of the how the date-only fields get handled because the time portion isn't always relevant (#2829848: Random test failure in DateRangeFieldTest will expand on this more if my hypothesis is correct).

Status: Needs review » Needs work

The last submitted patch, 5: 2829845-05.patch, failed testing.

The last submitted patch, 5: 2829845-05.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs work » Needs review

Back to NR based on latest run on fixed bots.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll since the tests have been converted to phpunit.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2829845-12.patch13.07 KB

Re-rolled.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Needs work 
+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -44,6 +44,7 @@ public function testDateField() {
+      $this->assertEqual($timezone, $this->config('system.date')->get('timezone.default'), 'Time zone set to ' . $timezone);

Sorry I missed this earlier. This should use the non-deprecated assertEquals method.


+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -633,6 +654,7 @@ public function testDefaultValue() {
+      $this->assertEqual($timezone, $this->config('system.date')->get('timezone.default'), 'Time zone set to ' . $timezone);

+++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
@@ -53,6 +53,7 @@ public function testDateRangeField() {
+      $this->assertEqual($timezone, $this->config('system.date')->get('timezone.default'), 'Time zone set to ' . $timezone);

And ditto on these 2.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs work » Needs review
FileSize
2829845-14.patch13.07 KB
interdiff-12-14.txt2.1 KB

Here we go.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good!

  • xjm committed 5ee33d6 on 8.5.x 
    Issue #2829845 by mpdonadio, Jo Fitzgerald, xjm, jhedstrom: Record...

  • xjm committed 5db3d42 on 8.4.x 
    Issue #2829845 by mpdonadio, Jo Fitzgerald, xjm, jhedstrom: Record...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -141,7 +150,10 @@ public function testDateField() {
-      $this->assertContains($expected, $output, SafeMarkup::format('Formatted date field using datetime_custom format displayed as %expected.', ['%expected' => $expected]));
+      $this->assertContains($expected, $output, new FormattableMarkup('Formatted date field using datetime_custom format displayed as %expected in %timezone.', [
+        '%expected' => $expected,
+        '%timezone' => $timezone,
+      ]));

These reformatted test assertions are easier to read, but most of them are completely out of scope. So are the removals of the deprecated SafeMarkup class.

Speaking of scope, an out-of-scope thing here would be that all of should probably use assertText() and remove the use of FormattableMarkup.

But, hey, this is a really good change and should be a big help when debugging with this test. I decided not to hold it back for fixing the scope. I'll just leave a reminder to make sure your other cleanups go in separate patches.

Committed to 8.5.x and backported to 8.4.x as a testing improvement. Thanks!

Status: Fixed » Closed (fixed)

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