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

Scope:
- All tests are in scope

CommentFileSizeAuthor
#20 2867304-20.patch10.02 KBmaxocub
#18 2867304-18.patch13.4 KBdawehner
#15 2867304-15.patch16.63 KBjofitz
#15 interdiff-13-15.txt1.49 KBjofitz
#13 interdiff-10-13.txt8.84 KBmaxocub
#13 2867304-13.patch10.06 KBmaxocub
#10 interdiff-8-10.txt2.49 KBmaxocub
#10 2867304-10.patch8.92 KBmaxocub
#8 2867304-8.patch9.12 KBmichielnugter
#7 2867304-7.patch24.14 KBmichielnugter
#6 interdiff-1-6.txt2.35 KBmaxocub
#6 2867304-6.patch137.95 KBmaxocub
#2 2867304-1.patch7.46 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Needs review » Needs work
StatusFileSize
new 2867304-1.patch7.46 KB

This patch has the conversions except for translatePostValues(). Therefore, not running the tests. Is there a 'replacement' for translatePostValues() ?

michielnugter’s picture

michielnugter commented
Issue tags: +phpunit initiative
maxocub’s picture

maxocub commented
Issue tags: +Baltimore2017

I don't see any replacement for translatePostValues() and it seems to be only used in two other tests.
We could move it to BrowserTestBase or just do the transformation directly in the test.

I did try moving it to the MigrateUpgradeTestBase, but then another problem arose:

There was 1 failure:

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\MigrateUpgrade6Test::testMigrateUpgrade
"Congratulations, you upgraded Drupal!" found
Failed asserting that false is true.

This seems to be failing because it doesn't wait until the upgrade is done before looking for that string, so it still on the progress page and it fails.
I don't know how to wait for an active progress to be done in BrowserTestBase, I'll look into that.

maxocub’s picture

maxocub commented
Status: Needs work » Postponed
Related issues: +#2862885: Batch: Convert system functional tests to phpunit

This is postponed on #2862885: Batch: Convert system functional tests to phpunit.
We will need the waitBatchProccessToEnd() method that's being implemented there.

maxocub’s picture

maxocub commented
Status: Postponed » Needs review
StatusFileSize
new 2867304-6.patch137.95 KB
new interdiff-1-6.txt2.35 KB

Now that #2862885: Batch: Convert system functional tests to phpunit is in, let's see if this is green.
I moved translatePostValues() into BrowserTestBase since a few other tests will probably be using it.

michielnugter’s picture

michielnugter commented
StatusFileSize
new 2867304-7.patch24.14 KB

Please use -M --find-copies with git diff for rolling patches, that will detect moves and copies and make the patch better reviewable. I attached a directly rerolled patch and will review from there.

michielnugter’s picture

michielnugter commented
StatusFileSize
new 2867304-8.patch9.12 KB 
  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1419,4 +1419,26 @@ protected function checkForMetaRefresh() {
+  /**
+   * Transforms a nested array into a flat array suitable for BrowserTestBase::drupalPostForm().
+   *
+   * @param array $values
+   *   A multi-dimensional form values array to convert.
+   *
+   * @return array
+   *   The flattened $edit array suitable for BrowserTestBase::drupalPostForm().
+   */
+  protected function translatePostValues(array $values) {
+    $edit = [];
+    // The easiest and most straightforward way to translate values suitable for
+    // BrowserTestBase::drupalPostForm() is to actually build the POST data string
+    // and convert the resulting key/value pairs back into a flat array.
+    $query = http_build_query($values);
+    foreach (explode('&', $query) as $item) {
+      list($key, $value) = explode('=', $item);
+      $edit[urldecode($key)] = urldecode($value);
+    }
+    return $edit;
+  }
+

    I don't think that adding this to the BrowserTestBase class is a good idea for now. We want to keep the magic methods down on the base class and promote testing like a user (i.e. directly manipulating the fields themselves).

    As the Simpletest version is only used twice in core right now I want to hold off on adding it to BrowserTestBase. It really should be fine in MigrateUpgradeTestBase,
    it really comes down to the same thing for the tests themselves. 

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1419,4 +1419,26 @@ protected function checkForMetaRefresh() {
diff --git a/test.html b/test.html

diff --git a/test.html b/test.html
new file mode 100644

new file mode 100644
index 0000000000..615febdd51

index 0000000000..615febdd51
--- /dev/null

--- /dev/null
+++ b/test.html

+++ b/test.html
+++ b/test.html
@@ -0,0 +1,176 @@

@@ -0,0 +1,176 @@
+‌‌$this->getSession()->getPage()->getHtml();
+‌
+<head>
+  <meta charset="utf-8">
+  <meta name="Generator" content="Drupal 8 (https://www.drupal.org)">
+  <meta name="MobileOptimized" content="width">
+  <meta name="HandheldFriendly" content="true">
+  <meta name="viewport" content="width=device-width, initial-scale=1.0">

    Whoops, now I made a mess of it ;) Ignore that one please! New patch attached (same as 6-7 but without the test.html..).

michielnugter’s picture

michielnugter commented
Status: Needs review » Needs work
maxocub’s picture

maxocub commented
Status: Needs work » Needs review
StatusFileSize
new 2867304-10.patch8.92 KB
new interdiff-8-10.txt2.49 KB

@michielnugter: Thanks for the git diff tip, it's much easier to read the patch now indeed.

maxocub’s picture

maxocub commented 
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -1,16 +1,15 @@
+use Drupal\Tests\BrowserTestBase;
 /**

Blank line missing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateAccessTest.php
similarity index 88%
rename from core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php

rename from core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
rename to core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php

Note: We should keep the old base classes around. These then should get a @trigger_error statement, have a look at https://www.drupal.org/files/issues/2870453-21.patch for an example.

maxocub’s picture

maxocub commented
Status: Needs work » Needs review
StatusFileSize
new 2867304-13.patch10.06 KB
new interdiff-10-13.txt8.84 KB

Added back the old base class with the @trigger_error and @deprecated.
Also corrected the missing blank line.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes

@maxocub thanks so much for working on this!

couple of nits:

  1. +++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error('\Drupal\migrate_drupal_ui\Tests\MigrateUpgradeTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase instead. See https://www.drupal.org/node/2867304.', E_USER_DEPRECATED);

    No need for the @see, git blame will lead anybody interested here anyway 

  2. +++ b/core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php
@@ -9,6 +11,9 @@
+ *   Use \Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase instead.

    Over 80 characters long, just moving 'Use' to the first line of the @deprecated statement should fix that.

And just wondering, do we consider the image files to be internal? If contrib is using these files to test migrations, those test will be broken now, but not sure how we deprecate an image file.....
I'm fine with moving them, but I'm not as BC conscious as some others

jofitz’s picture

jofitz
he/him
commented
StatusFileSize
new interdiff-13-15.txt1.49 KB
new 2867304-15.patch16.63 KB

Fixed @Lendude's nits from #14.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
@@ -0,0 +1,229 @@
+abstract class MigrateUpgradeTestBase extends BrowserTestBase {

Just out of curiousity, do you understand why git doesn't pick up this file as being renamed?

jofitz’s picture

jofitz
he/him
commented

MigrateUpgradeTestBase.php is not picked up as being renamed because the original remains and is deprecated while a copy (new file) is created in the new location.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2867304-18.patch13.4 KB

Well, git should still be able to deal with that. Here is what I get as part of my git configuration ...
I guess this bit of configuration: 

[diff]
  renames = copies
  renameLimit = 1500

makes the difference?

Status: Needs review » Needs work

The last submitted patch, 18: 2867304-18.patch, failed testing. View results

maxocub’s picture

maxocub commented
Status: Needs work » Needs review
StatusFileSize
new 2867304-20.patch10.02 KB

@dawehner: Yes, that's a better diff. Here's a new patch without the unrelated changes though.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thanks a ton @maxocub!

The last submitted patch, 2: 2867304-1.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • catch committed 62fa797 on 8.4.x 
    Issue #2867304 by maxocub, michielnugter, Jo Fitzgerald, dawehner,...

Status: Fixed » Closed (fixed)

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

maxocub’s picture

maxocub commented