Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Needs review » Needs work
FileSize
7.46 KB

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

michielnugter’s picture

Issue tags: +phpunit initiative
maxocub’s picture

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

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

Status: Postponed » Needs review
FileSize
137.95 KB
2.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

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

  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

Status: Needs review » Needs work
maxocub’s picture

Status: Needs work » Needs review
FileSize
8.92 KB
2.49 KB

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

maxocub’s picture

+++ 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

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

Status: Needs work » Needs review
FileSize
10.06 KB
8.84 KB

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

Lendude’s picture

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

Jo Fitzgerald’s picture

Fixed @Lendude's nits from #14.

dawehner’s picture

+++ 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?

Jo Fitzgerald’s picture

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

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

Status: Needs work » Needs review
FileSize
10.02 KB

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

dawehner’s picture

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

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