Problem/Motivation

DateTimeItemTest is not a valid test. The formats used for the field configuration do not match each other. In addition, the actual values stored aren't valid per the defined formats.

Proposed resolution

Fix the test.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2832264-02.patch8.16 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Assigned: mpdonadio » Unassigned
Status: Active » Needs review
FileSize
2832264-02.patch8.16 KB

Here is a port of the test changes from #2824717-15: Add a format constraint to DateTimeItem to provide REST error message w/o any of the constraint tests. Passes locally.

mpdonadio’s picture

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

The diff on this is a little hard to read to see what is really wrong. It is best to read the file before and after the patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented

This is looking good. Just one question really:

  1. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
@@ -27,33 +42,74 @@ protected function setUp() {
+    $this->assertTrue($entity->field_datetime[0] instanceof FieldItemInterface, 'Field item implements interface.');
...
+    $this->assertEqual($entity->field_datetime[0]->value, $value);
...
+    $this->assertEqual($entity->field_datetime->value, $new_value);
...
-    $value = '2014-01-01T20:00:00Z';

    Do we need to check the [0] bit? If so, should we check it below as well where checking the updated value? 

  2. +++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
@@ -64,12 +120,12 @@ public function testDateTimeItem() {
-    $new_value = $this->randomMachineName();
+    $new_value = '2016-11-04';

    Lol, machine name for date!

mpdonadio’s picture

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

#4-1, if you look at the before/after for the whole class, that hunk makes more sense. The current test does that; the changes mirror how the existing testing is done, but make them valid and creates separate test methods for date+time and date-only variants. The first blip with [0] is testing that everything is created properly w/ the right interfaces (the list interface and the item interface) and that you can set both ways. Below, the blip w/o the [0] is just testing that you can change the values.

I'd be fine w/ adding some additional assertions, but I think they are overkill. We don't test all of the ways we can set/get field values because that is not the responsibility of the class under test, the parent class handles that magic.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Priority: Major » Normal
Issue tags: +DrupalCampNJ2017

I am downgrading this. It is not major, and the parent issue has been committed.

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.

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

re #5 thanks for the clarification. I think this is good to go.

xjm’s picture

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

The 8.2.x retest failed and is no longer valid anyway, so I attached an 8.3.x test run.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
@@ -83,11 +139,15 @@ public function testDateTimeItem() {
-    $value = '2014-01-01T20:00:00Z';
+    $value = '2014-01-01T20:00:00';

Hm why are we removing the Z here? Is this what's being discussed in #4 and #5?

Everything else in the patch seemed like valid refactoring of and additions to the test coverage; just that caught me out.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

NR for #10 (at least a confirmation that it's intentional and brief explanation of why). I read the updated test as a whole and did not see an explicit reason one way or the other (though everything else looked good).

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Related issues: +#2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken, +#2824717: Add a format constraint to DateTimeItem to provide REST error message

#10, we do not store the timezone with the timestamp. See #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken. The format for date+time is Y-m-d\TH:i:s. So the value that we use in the test should match. The fact that it will currently work with the 'Z' should be addressed by #2824717: Add a format constraint to DateTimeItem to provide REST error message (a lot of the test changes are unneeded in that patch if this one lands first, I would almost call that one soft-blocked by this).

mpdonadio’s picture

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

And as another note, I would really prefer for this to land before #2824717: Add a format constraint to DateTimeItem to provide REST error message. The constraint issue is going to be hard to get done for two reasons (getting rid of form validation / making work with daterange, and also because of when constraints get called when saving from widgets and how the datelist widget works). I think having a valid test is more important right now.

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

I think #12 and #13 have addressed #10, so back to RTBC.

  • xjm committed 88a8787 on 8.4.x 
    Issue #2832264 by mpdonadio, jhedstrom: DateTimeItemTest is not valid

  • xjm committed 4b181ff on 8.3.x 
    Issue #2832264 by mpdonadio, jhedstrom: DateTimeItemTest is not valid

(...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Okay, great, thanks for the clarification! Committed to 8.4.x and backported to 8.3.x as a test improvement to an individual test.

Status: Fixed » Closed (fixed)

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