Follow-up to #2161337: Add a Date Range field type with support for end date

Problem/Motivation

  • DateTimeFieldTest and DateRangeFieldTest have a lot of similar/duplicate code.
  • When in the future, test coverage is added/changed in DateTimeFieldTest, chances are a similar change will be needed in DateRangeFieldTest too.

Proposed resolution

Create a base class for the two, so that some future changes only need to be made in one place.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

mpdonadio’s picture

We may want to merge this with #2780063: Convert web tests to browser tests for datetime and datetime_range modules and do both at the same time. This may depend on how some of the new traits on BTB go. There are still a bunch missing to make this an easy conversion.

xjm’s picture

xjm’s picture

Regarding #2, I recommend not blocking things on a BTB conversion. See my feedback on the BTB meta for why we can't convert individual module tests to BTB at this time: #2735005-69: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
15.92 KB

Patch (not converted to BTB).

Status: Needs review » Needs work

The last submitted patch, 5: 2786583-5.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2786583-5.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
15.89 KB
428 bytes

Forgot the @group in base class.

The last submitted patch, 5: 2786583-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2786583-9.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
jhedstrom’s picture

This is looking really great! Thanks @claudiu.cristea!

I'm not clear on if this can go into 8.2 or not, but we'll leave it there for now.

A couple of small changes and it is looking good.

  1. +++ b/core/modules/datetime/src/Tests/DateTestBase.php
    @@ -0,0 +1,181 @@
    +      // A timezone with an offset greater than UTC+12 is used.
    

    While we're refactoring this, let's remove this incorrect comment (this goes back to when the timezone loop was added, but the comment was never cleaned up).

  2. +++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
    @@ -88,56 +37,17 @@ class DateRangeFieldTest extends WebTestBase {
    +    $this->dateFormatter = $this->container->get('date.formatter');
    

    Can we add this property to the base class instead, and have the datetime field test use that as well? (It currently uses \Drupal::service('date.formatter').)

claudiu.cristea’s picture

FileSize
18.77 KB
5.94 KB

Thank you @jhedstrom

Here are the changes.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, thanks @claudiu.cristea!

I am still uncertain which branch it can go into.

jhedstrom’s picture

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

Bumping up to 8.3.x, as it will need to go in there before it goes into 8.2.x (if it can).

effulgentsia’s picture

Version: 8.3.x-dev » 8.2.x-dev
Issue tags: +rc eligible

@xjm pointed me to https://www.drupal.org/core/d8-allowed-changes#rc which says testing improvements (i.e., patches that solely touch tests) can go into RCs (and therefore also into betas), so tagging as such, and all 8.x issues are presumed to need to be committed to the latest branch first, and then cherry-picked to the issue's version, so setting that accordingly.

She also mentioned that not every testing improvement is allowed into betas/rcs/patch-releases. For example, #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) is too disruptive. But I don't see anything that disruptive in this patch.

  • xjm committed f03979c on 8.3.x
    Issue #2786583 by claudiu.cristea, jhedstrom, effulgentsia: Create a...

  • xjm committed 1528b56 on 8.2.x
    Issue #2786583 by claudiu.cristea, jhedstrom, effulgentsia: Create a...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice cleanup! Committed f03979c and pushed to 8.3.x and 8.2.x. Thanks!

Fixed the following coding standards violation on commit:

FILE: .../git/maintainer/core/modules/datetime/src/Tests/DateTestBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 111 | ERROR | [x] Expected 1 space after "="; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
diff --git a/core/modules/datetime/src/Tests/DateTestBase.php b/core/modules/datetime/src/Tests/DateTestBase.php
index 4c9d01c..0f1277e 100644
--- a/core/modules/datetime/src/Tests/DateTestBase.php
+++ b/core/modules/datetime/src/Tests/DateTestBase.php
@@ -108,7 +108,7 @@ protected function setUp() {
   protected function createField() {
     $field_name = Unicode::strtolower($this->randomMachineName());
     $type = $this->getTestFieldType();
-    $widget_type = $formatter_type =  $type . '_default';
+    $widget_type = $formatter_type = $type . '_default';
 
     $this->fieldStorage = FieldStorageConfig::create([
       'field_name' => $field_name,

Status: Fixed » Needs work

The last submitted patch, 14: 2786583-14.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Fixed

Back to fixed :)

Status: Fixed » Closed (fixed)

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