Random test failure in DateRangeFieldTest

99% sure I know what is happening here. Will post a patch later.

#8, is there anything in particular about that article that you think may be causing this problem?

OK, I think this is what is happening. In working on #2823847: DateRange formatters should compare rendered dates instead of raw timestamps, I noticed that when in the daterange formatters, the timestamps in this hunk

        /** @var \Drupal\Core\Datetime\DrupalDateTime $start_date */
        $start_date = $item->start_date;
        /** @var \Drupal\Core\Datetime\DrupalDateTime $end_date */
        $end_date = $item->end_date;

        if ($start_date->format('U') !== $end_date->format('U')) {
          // ...
        }

weren't always the same (look at some of fails on the earlier patches) for date-only items. The Y/m/d portions where the same, but the time wasn't. This is because we are deferring the call to `datetime_date_default_time()` to the build methods (that method needs to go; followup to come). I suspect we are having a second-rollover between when the start and end computed values are occurring, and that is causing the spurious.

This patch adds the default time logic to DateTimeComputed so that the computed value on the field always has the proper time set for date-only items, regardless of where you are using them.

Two notes:

1. Think we also need this in the DateTimeComputed::setValue() method, but I am seeing some weirdness there that I don't quite understand.

2. In the hunk above, we can't compare timestamps in the formatters. Since the field is based around \DateTime at the core, values can be outside the Unix timestamp range. The referenced issue handles this by accident, but that also introduces a change that I don't think we want in 8.2.x. My next patch will have a fix for this.

Here is an updated patch to the formatters to not use timestamps, but see also the recent comments on #2795489: 2038 bug with PHP timestamps on 32-bit systems - warn users?.

Calgon, take me away!

And regards to the second fail that @xjm couldn't reproduce, it is essentially the same problem: rendering out a range w/o the separator when the dates are the same. That particular assertion is making sure hook_entity_prepare_view() can run and is attaching the attribute to the proper element. When the range is displayed, it is the div that wraps the two time elements. When it is the same, and no separator is present, it attaches to the time element directly. See #2811725: Error when render Datetime Range field: Error: Unsupported operand types.

Back to NR based on latest runs on fixed bots.

This will exacerbate the hypothesis of the problem is rollover between seconds when the computed datetime objects are created. It has high failure rate for me locally, but really lengthens the test time.

#37, yeah, should use the item constant there (see also #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module).

#36-1, yup, that should go.

#36-2, long answer here. The 12:00 thing is a legacy thing that has been in core for a long time. We opened an issue to #2830094: Deprecate and remove usages of datetime_date_default_time()., but haven't figured out how we want to remove all usages.

The 12:000 UTC time is chosen because it will work with the date-only portion for most cases, except around the edge cases. In other words, when it is noon UTC it is the same day just about everywhere. We have to pick something so it is consistent, and that is the most reasonable option. We fixed a bunch of those bugs (#2778083: Default value for Date-Only fields is broken when UTC date is different than user current date, #2739290: UTC+12 is broken on date only fields), and in the process now do that TZ loop whenever we are testing date-only things (may need to add some more, or they are in-progress patches). Using 12:00 in the patch is consistent for how we use this in core, we are just setting it earlier in the process. I suspect doing this in the computed item will eliminate the need for many (or most) of the non-test uses of datetime_date_default_time().

I would be happy with touching up #10 with the feedback above and handling #36-2 with a comment above the ->setTime() with why it is needed. #12 is slightly different than #2823847: DateRange formatters should compare rendered dates instead of raw timestamps; the later is a BC break, the former uses the storage strings in the formatter comparison instead of timestamps in addition to the change in the computed item. #10 is the least invasive way to fix the known problem. I am hesitant to say we are bug-free, but until we have other fails to look at, I think potential problems are speculation.

I'll post a new patch in a bit.

Updated patch for commit review. Interdiff is against #10, which was the starting point.

#40, should be pretty easy to do in new DateTimeItemTest test. Will post tomorrow.

#41, those lines adjust the expected values used in the tests. DrupalDateTime objects get created and formatted to mimic the expected formatter output; there is never a trip to the fields or the computed objects. That is a refactor that I would like to do in #2830094: Deprecate and remove usages of datetime_date_default_time(). when we can nuke that method. In the tests, I'm hoping to centralize that behavior in a DateTestBase::massageDateObject() method. So, for now they need to stay.

And I just remembered that DateTimeItemTest is invalid. See the changes to it in #2824717: Add a format constraint to DateTimeItem to provide REST error message that are unrelated to the constraint.

@xjm, can you look at that patch and let me know how you want to proceed?

#44 testing this in DateTimeItemTest (which is where it really belongs) is problematic b/c that test is invalid. Right now, if you use the Entity API, you can store the wrong format for the datetime_type, and the test doesn't even use the proper format. This never got noticed until we tried adding the constraint. We can get that issue in, create a separate issue to fix that test, or create a new test class for this issue.

Patch is on #2832264: DateTimeItemTest is not valid.

To be honest I'm not entirely convinced by choosing 12:00:00 UTC instead of 00:00:00 UTC. If you think about it at least the start of the unix timestamp was not 12:00:00 UTC, and days usually start at midnight :)

+++ b/core/modules/datetime/src/DateTimeComputed.php
@@ -40,14 +41,27 @@ public function getValue($langcode = NULL) {
+        // time will be set from the current time instead. For consistency, we
+        // we the time to 12:00:00 UTC. This is use so that the local date

double we in this sentence :) This is also related to #2842409: Selecting the same day in a date between filter returns no results and many more issues.

Adjusted the comment in #49.

12:00:00 UTC is what it does now, for better or worse. It we make it 00:00:00 UTC, then we have other problems. Changing it, though, would be a BC break. We can discuss it in #2830094: Deprecate and remove usages of datetime_date_default_time()..

Shouldn't the fix \Drupal\Component\Datetime\DateTimePlus::createFromFormat() - shouldn't that set the time consistently using the same logic if the supplied format does not have a time?

Also even if we don't do that I think this is testable.

Also \Drupal\datetime_range\Plugin\Field\FieldWidget\DateRangeWidgetBase::massageFormValues() seems to be doing something similar too.

#51, re DateTimePlus(), it would be practically impossible to determine whether a supplied format contains time or not, since it (via \DateTime::createFromFormat) can work with format, which may even contain a portion time, and handle in a sane way.

In the field items we explicitly only with with the two storage formats we have defined.

#52, not sure which part you are thinking of. We use datetime_date_default_time() for date-only items (which set 12:00:00 UTC). These

$start_date->setTime(0, 0, 0);
$end_date->setTime(23, 59, 59);

are for the all-day variant of daterange, because we set the date to be midnight-to-midnight to represent something that lasts all day. We also have an issue to nuke datetime_date_default_time(), #2830094: Deprecate and remove usages of datetime_date_default_time().. I think if we consolidate this approach in the computed item in the value, we can get rid of nearly all of the the non-test uses. For the uses in the test, we would move the logic to a DateWhateverTestBase::massageTestDate() to handle this and other duplicated code.

I think the test approach in #40 would work, assuming we can fix to mess in #2832264: DateTimeItemTest is not valid.

When I was debugging this, I was pretty confident this was the best approach to address the immediate need of random failures during second rollovers.

Here's a test that proves the above approach will work.

#54 is essentially what I wanted to add to DateTimeItemTest, just in a slightly different way. Reworking this to be DateRangeItemTest and renaming a few other things is probably good for the long term. I'll post some tweaks later.

No substantial changes from #54, just a little cleanup to give this a proper home and a test class to build off of (eg, start to mirror what DateTimeItemTest should be doing).

Pending review, I think this is committable w/o the cleanup in #2832264: DateTimeItemTest is not valid and is sufficient to address the critical / random fail. We have other (non-critical) issues to handle the rest of comments raised in the issue. It also applies cleanly to 8.3.x.

Nice changes to the test +1.

Given that I've only added test coverage to the issue I think I'm in a good position to RTBC this. I also think we can have a followup to remove some of \Drupal\datetime_range\Plugin\Field\FieldWidget\DateRangeWidgetBase::massageFormValues().

Opened the follow-up at #2846963: Clean up DateRangeWidgetBase::massageFormValues()..

Cleaned up the following two issues on commit.

1. +++ b/core/modules/datetime/src/DateTimeComputed.php
   @@ -40,14 +41,28 @@ public function getValue($langcode = NULL) {
   +        // that the local date  portion is the same, across nearly all time
   

   extra space, fixed on commit.

2. +++ b/core/modules/datetime/src/DateTimeComputed.php
   @@ -40,14 +41,28 @@ public function getValue($langcode = NULL) {
   +        // @todo Update comment and/or code when per the chosen solution in
   

   Stray 'when'.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x, thanks!