Problem/Motivation

See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Status: Active » Needs review
FileSize
869 bytes

May be premature since we have some major work going on with this, but want to start to identify what this needs for conversion. Being able to mock some of the classes for consistent tests will be a huge benefit for flushing out any remaining timezone problems.

Here is just the base class change. It is going to fail b/c the xpath and $this::url.

Note to anyone else working on this, the `renames = copies` bit in https://www.drupal.org/node/1542048 is important to make sure we have minimal (ha!) rebase / merge problems once other patches start landing.

Status: Needs review » Needs work

The last submitted patch, 2: 2780063-02.patch, failed testing.

xjm’s picture

See: #2735005-69: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

I would recommend postponing this, or using it to contribute BC layers upstream to BTB.

mpdonadio’s picture

Title: Convert web tests to browser tests for datetime module » Convert web tests to browser tests for datetime and datetime_range modules
Status: Needs work » Postponed
Related issues: +#2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests

Doing some housekeeping, and postponing this on #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests. Also note that they are currently in the exclusion list in #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits.

When that lands, I think we need to make the decision on how to proceed with this. I think that is the only main blocking issue (also need to replace $this->url in a few places with the one from the session).

DateTimeFieldTest and DateRangeFieldTest are monsters. I think at the very least, we need to refactor them to split out the item/field, widget, and formatter tests into separate classes, and make the decision as to whether to use UTB, KTB, or BTB. And at the same time, clean up some of the kernel-style things that are being done in UI tests (note how in #2811725: Error when render Datetime Range field: Error: Unsupported operand types how this caused something to slip through the cracks).

mpdonadio’s picture

Status: Postponed » Needs review
FileSize
14.72 KB

Here is where we stand with rename + #2784537: Add legacy assertFieldByXPath()/assertNoFieldByXPath() method for browser tests + `$this->url => $this->getSession()->getCurrentUrl()`.

Not sure why we aren't getting past the $this->drupalLogin() here.

Status: Needs review » Needs work

The last submitted patch, 6: 2780063-06.patch, failed testing.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

mpdonadio’s picture

So, this is essentially blocked on drupalPostAjaxForm() support. We also need to figure out what to do with DateTestBase::renderTestEntity(), in particular the setRawContent() call.

mpdonadio’s picture

Issue tags: +DrupalCampNJ2017

Keep forgetting to tag issues.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
8.73 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 11: 2780063-11.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
44.74 KB
52.53 KB

$this->renderTestEntity() can no longer work in the same manner under BTB so this assertions after running this needed to be refactored (similar to #2763013: Convert web tests to browser tests for link module).

I'm open to suggestions of a simpler method for solving the test failures, but I sure can't think of one (and I've spent a long time thinking about it)!

mpdonadio’s picture

#13 see https://www.drupal.org/node/2809181#comment-11954258 for an idea I had about how to rework $this->renderTestEntity().

Your idea looks better, though, at quick glance.

mpdonadio’s picture

Status: Needs review » Needs work

Green patch! Sa-weet!

Partial review, a lot of the same comment apply to the DateRange version.

  1. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -115,8 +115,8 @@ public function testDateField() {
    -              $this->renderTestEntity($id);
    -              $this->assertFieldByXPath('//time[@datetime="' . $expected_iso . '"]', $expected, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
    +              $output = $this->renderTestEntity($id);
    +              $this->assertContains($expected_iso, $output, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
    

    I'm worried we are losing coverage here. The XPath assertion made sure the time element was actually rendered out. This justs checked that the ISO string is there.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -215,7 +215,7 @@ public function testDatetimeField() {
    -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend//text()', $field_name, 'Fieldset and label found');
    +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_name, 'Fieldset and label found');
    

    Is this needed?

  3. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -259,8 +259,8 @@ public function testDatetimeField() {
    -            $this->renderTestEntity($id);
    -            $this->assertFieldByXPath('//time[@datetime="' . $expected_iso . '"]', $expected, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
    +            $output = $this->renderTestEntity($id);
    +            $this->assertContains($expected_iso, $output, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
    

    Ditto about time.

  4. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -368,7 +368,7 @@ public function testDatelistWidget() {
    -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend//text()', $field_name, 'Fieldset and label found');
    +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_name, 'Fieldset and label found');
         $this->assertFieldByXPath('//fieldset[@aria-describedby="edit-' . $field_name . '-0--description"]', NULL, 'ARIA described-by found');
    

    Ditto about needed.

  5. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -381,7 +381,7 @@ public function testDatelistWidget() {
    -    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
    +    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");
    

    Hmmm. I think I am OK with this change. Not really sure why this needed Ajax in the first place.

  6. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -407,7 +407,7 @@ public function testDatelistWidget() {
    -    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
    +    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");
         $this->assertFieldByXPath($xpathIncr, NULL, 'Increment element found for Date and time.');
    

    Ditto about Ajax.

  7. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -577,16 +577,16 @@ public function testAlldayRangeField() {
    -  public function testDatelistWidget() {
    +  public function xtestDatelistWidget() {
    

    Comment out test :) Happens in a bunch of places in the DateRange test.

Overall, this looks great. Mainly worried about losing coverage on the time element.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
14.27 KB
52.67 KB
  1. I also have concerns about the loss of coverage, so I've tried something a little better: $expected_markup = '<time datetime="' . $expected_iso . '" class="datetime">' . $expected . '</time>'; so at least it is now looking for a rendered element.
  2. Removing //text() avoids the error
    Call to undefined method DOMText::hasAttribute()

    (see #11)

  3. See 1.
  4. See 2.
  5. This change has been applied to some other similar patches, e.g. #2747167: Convert first part of web tests of views_ui (admittedly that is not a complete patch yet)
  6. See 5.
  7. Ooops! My mistake. Fixed now.
klausi’s picture

Status: Needs review » Postponed
  1. +++ b/core/modules/datetime/tests/src/Functional/DateTestBase.php
    @@ -161,9 +161,7 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
         $build = $display->build($entity);
    -    $output = $this->container->get('renderer')->renderRoot($build);
    -    $this->setRawContent($output);
    -    $this->verbose($output);
    +    return (string) $this->container->get('renderer')->renderRoot($build);
    

    we need to update the docs for this function to have an @return tag. See also the link module conversion.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -67,7 +67,7 @@ public function testDateField() {
    -      preg_match('|entity_test/manage/(\d+)|', $this->url, $match);
    +      preg_match('|entity_test/manage/(\d+)|', $this->getSession()->getCurrentUrl(), $match);
    

    we can just use ->getUrl() here.

  3. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -235,7 +236,7 @@ public function testDatetimeField() {
    -    preg_match('|entity_test/manage/(\d+)|', $this->url, $match);
    +    preg_match('|entity_test/manage/(\d+)|', $this->getSession()->getCurrentUrl(), $match);
    

    same here.

  4. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -415,23 +417,23 @@ public function testDatelistWidget() {
    -    $this->assertOptionByText("edit-$field_name-0-value-year", t('Year'));
    +    $this->assertOption("edit-$field_name-0-value-year", t('Year'));
    

    we need to open an issue to add assertOptionByText() to AssertLegacyTrait for better browser test compatibility. Then we don't have to change these lines.

Postponing this until we have #2862470: Add assertOptionByText() to AssertLegacyTrait for better browser test compatibility.

GoZ’s picture

Status: Postponed » Needs work
GoZ’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysSeville
FileSize
51.46 KB
4.22 KB
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -67,7 +67,7 @@ public function testDateField() {
    -      preg_match('|entity_test/manage/(\d+)|', $this->url, $match);
    +      preg_match('|entity_test/manage/(\d+)|', $this->getSession()->getCurrentUrl(), $match);
    

    you can use $this->getUrl() here.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -444,7 +446,7 @@ public function testDatelistWidget() {
    -    preg_match('|entity_test/manage/(\d+)|', $this->url, $match);
    +    preg_match('|entity_test/manage/(\d+)|', $this->getSession()->getCurrentUrl(), $match);
    

    same here, let's use getUrl() everywhere.

GoZ’s picture

Status: Needs work » Needs review
FileSize
53.47 KB
1.58 KB

Right, i only replaced the one you mentioned.

boaloysius’s picture

patch #19 had 12 matches for $this->getSession()->getCurrentUrl().

dawehner’s picture

+++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
@@ -129,8 +130,8 @@ public function testDateField() {
-      $this->renderTestEntity($id);
-      $this->assertText($expected, SafeMarkup::format('Formatted date field using plain format displayed as %expected.', ['%expected' => $expected]));
+      $output = $this->renderTestEntity($id);
+      $this->assertContains($expected, $output, SafeMarkup::format('Formatted date field using plain format displayed as %expected.', ['%expected' => $expected]));

I really like how this makes the test a bit more explicit.

+++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
@@ -622,7 +632,7 @@ public function testDatelistWidget() {
 
     // Click on the widget settings button to open the widget settings form.
-    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
+    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");
     $xpathIncr = "//select[starts-with(@id, \"edit-fields-$field_name-settings-edit-form-settings-increment\")]";
     $this->assertNoFieldByXPath($xpathIncr, NULL, 'Increment element not found for Date Only.');

@@ -656,7 +666,7 @@ public function testDatelistWidget() {
     // Click on the widget settings button to open the widget settings form.
-    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
+    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");

I wonder this could be a JavascriptTestBase test instead. Maybe you could open up a new follow up for that.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

I think the conversions for drupalPostAjaxForm() are fine because we did not test javascript functionality anyway. We are just testing the HTML of the response, which is the same when posting the form normally and via AJAX.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Reviewed & tested by the community » Needs review

I am 98% sure that this is good, but I want to give this a close study to make sure we aren't regressing anything with xpath test changes.

We also need to create a f/up plan to figure out how to make these more managble. 1000+ line test classes are not good.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

OK, for my own sanity, I verified that the tests are actually being run, https://dispatcher.drupalci.org/job/drupal_patches/8878/consoleFull

  1. +++ b/core/modules/datetime/tests/src/Functional/DateTestBase.php
    @@ -153,6 +153,9 @@ protected function createField() {
    +   *
    +   * @return string
    +   *   The rendered HTML output.
    

    Because of needing to rework what renderTestEntity does in order for a minimal conversion, this is an in-scope-change.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTestBase.php
    @@ -161,9 +164,7 @@ protected function renderTestEntity($id, $view_mode = 'full', $reset = TRUE) {
    -    $output = $this->container->get('renderer')->renderRoot($build);
    -    $this->setRawContent($output);
    -    $this->verbose($output);
    +    return (string) $this->container->get('renderer')->renderRoot($build);
    

    This could potentially be done with some Mink shenanigans to do the exact same thing, but I this this is a necessary change. The link.module conversion is also doing the same thing.

  3. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -115,8 +115,9 @@ public function testDateField() {
    -              $this->renderTestEntity($id);
    -              $this->assertFieldByXPath('//time[@datetime="' . $expected_iso . '"]', $expected, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
    +              $output = $this->renderTestEntity($id);
    +              $expected_markup = '<time datetime="' . $expected_iso . '" class="datetime">' . $expected . '</time>';
    +              $this->assertContains($expected_markup, $output, SafeMarkup::format('Formatted date field using %value format displayed as %expected with %expected_iso attribute.', ['%value' => $new_value, '%expected' => $expected, '%expected_iso' => $expected_iso]));
                   break;
    

    Not totally happy with this, but scoping this as a conversion with minimal refactoring, I think this is a necessary change. It make this a little more brittle, but I don't see a better alternative.

  4. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -1,12 +1,12 @@
     use Drupal\Component\Render\FormattableMarkup;
     use Drupal\Component\Utility\Unicode;
     use Drupal\Core\Datetime\DrupalDateTime;
     use Drupal\Core\Datetime\Entity\DateFormat;
    -use Drupal\datetime\Tests\DateTestBase;
    +use Drupal\Tests\datetime\Functional\DateTestBase;
     use Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem;
     use Drupal\entity_test\Entity\EntityTest;
     use Drupal\field\Entity\FieldConfig;
    

    Nit that can be fixed on commit, should be alphabetical.

  5. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -434,7 +440,7 @@ public function testAlldayRangeField() {
    -    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend//text()', $field_name, 'Fieldset and label found');
    +    $this->assertFieldByXPath('//fieldset[@id="edit-' . $field_name . '-0"]/legend', $field_name, 'Fieldset and label found');
    

    See #16.

I went through this pretty carefully, and think we have as close to a one-to-one conversion as we are going to get. These tests work, but are a big mess. Adding Needs Followup so we can plan out how to refactor these now that the BTB conversion is done. Will make that issue later.

This is RTBC from my perspective.

Lendude’s picture

similarity index 94%
rename from core/modules/datetime/src/Tests/DateTestBase.php

rename from core/modules/datetime/src/Tests/DateTestBase.php
rename to core/modules/datetime/tests/src/Functional/DateTestBase.php

Are we just (re)moving base classes? Not deprecating them and making a new BrowserTestBase version? This would break any contrib testsuite building their tests on these Webtestbase base classes.

Follow up needed for the Views based test in Datetime, as far as I can see they should be converted to kernel tests so opened up #2865992: Convert Datetime module Views tests to Kerneltest and rolled that conversion.

mpdonadio’s picture

Status: Needs review » Needs work

The last submitted patch, 28: 2780063-28.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review

Retriggered b/c the JSTB intermittents, https://www.drupal.org/pift-ci-job/637557

Lendude’s picture

+++ b/core/modules/datetime/src/Tests/DateTestBase.php
@@ -0,0 +1,186 @@
+namespace Drupal\datetime\Tests;
+
+use Drupal\Component\Utility\Unicode;

Missing the trigger_error under the namespace as described in the Drupal core deprecation policy.

mpdonadio’s picture

FileSize
51.31 KB
989 bytes

Here we go.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@mpdonadio looks great.

Applied the patch and everything that needs to be moved, gets moved.

Back to RTBC.

alexpott’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work

Committed 5e8c04d and pushed to 8.4.x. Thanks!

The patch needs re-rolling for 8.3.x.

  • alexpott committed 5e8c04d on 8.4.x
    Issue #2780063 by mpdonadio, Jo Fitzgerald, GoZ, boaloysius, klausi,...
boaloysius’s picture

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
51.63 KB
$ git checkout -b 2780063-37 8.3.x
$ git apply --index -3 2780063-32.patch
error: patch failed: core/modules/datetime/src/Tests/DateTimeFieldTest.php:1
Falling back to three-way merge...
Applied patch to 'core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php' with conflicts.
error: patch failed: core/modules/datetime_range/src/Tests/DateRangeFieldTest.php:165
Falling back to three-way merge...
Applied patch to 'core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php' with conflicts.
U core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
U core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php

The conflict is from a test change in #2775669: Clean up redundant methods in datetime field formatters b/c changing the daterange formatters from #plain_text to #markup. DateRangeFieldTest is going to fail here, but this is to show clean change.

mpdonadio’s picture

And here is that hunk removed.

The last submitted patch, 37: 2780063-37.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -138,8 +140,18 @@ public function testDateField() {
    +      // Test that allowed markup in custom format is preserved and XSS is
    +      // removed.
    +      $this->displayOptions['settings']['date_format'] = '\\<\\s\\t\\r\\o\\n\\g\\>m/d/Y\\<\\/\\s\\t\\r\\o\\n\\g\\>\\<\\s\\c\\r\\i\\p\\t\\>\\a\\l\\e\\r\\t\\(\\S\\t\\r\\i\\n\\g\\.\\f\\r\\o\\m\\C\\h\\a\\r\\C\\o\\d\\e\\(\\8\\8\\,\\8\\3\\,\\8\\3\\)\\)\\<\\/\\s\\c\\r\\i\\p\\t\\>';
    +      entity_get_display($this->field->getTargetEntityTypeId(), $this->field->getTargetBundle(), 'full')
    +        ->setComponent($field_name, $this->displayOptions)
    +        ->save();
    +      $expected = '<strong>' . $date->format('m/d/Y') . '</strong>alert(String.fromCharCode(88,83,83))';
    +      $output = $this->renderTestEntity($id);
    +      $this->assertContains($expected, $output, new FormattableMarkup('Formatted date field using daterange_custom format displayed as %expected.', ['%expected' => $expected]));
    

    I always like additional test coverage.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -370,7 +383,7 @@ public function testDatelistWidget() {
         // Click on the widget settings button to open the widget settings form.
    -    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
    +    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");
    
    @@ -396,7 +409,7 @@ public function testDatelistWidget() {
         // Click on the widget settings button to open the widget settings form.
    -    $this->drupalPostAjaxForm(NULL, [], $field_name . "_settings_edit");
    +    $this->drupalPostForm(NULL, [], $field_name . "_settings_edit");
         $this->assertFieldByXPath($xpathIncr, NULL, 'Increment element found for Date and time.');
    

    Don't we loose some level of test coverage here?

mpdonadio’s picture

#40, up in #24 @klausi said

I think the conversions for drupalPostAjaxForm() are fine because we did not test javascript functionality anyway. We are just testing the HTML of the response, which is the same when posting the form normally and via AJAX.

I agree with this; and this is just the backport patch now. The followup that I need to make is to break up and refactor DateTimeFieldTest and DateRangeFieldTest into more manageable chunks, and see if we can refactor some of the common bits (one or two other issues will make this easier), and also split out some of these into KTB tests. When we do that, we can add some JSTB where needed.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Yeah I think this is fine for the backport, we don't want the tests to get out of sync right?

@mpdonadio a follow up like that would be really great, date tests are hard enough without having to go through these massive files :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for working on the backport - and yeah out-of-sync tests are really tricky.

Committed 76a4428 and pushed to 8.3.x. Thanks!

  • alexpott committed 76a4428 on 8.3.x
    Issue #2780063 by mpdonadio, Jo Fitzgerald, GoZ, boaloysius, Lendude,...

Status: Fixed » Closed (fixed)

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