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

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

None.

CommentFileSizeAuthor
#74 2757007-74.patch4.9 KBtameeshb
#63 2757007-63.patch4.92 KBtameeshb
#63 interdiff-56-63.txt546 bytestameeshb
#62 2757007-62.patch5.41 KBtameeshb
#62 interdiff-56-62.txt1.16 KBtameeshb
#60 interdiff-2757007-57-60.txt2.61 KBhimanshu-dixit
#60 2757007-60.patch75.65 KBhimanshu-dixit
#57 interdiff-53-56.txt657 bytestameeshb
#57 2757007-56.patch4.12 KBtameeshb
#56 interdiff-52-54.txt375 bytestameeshb
#56 2737805-54.patch6.9 KBtameeshb
#53 2757007-53.patch4.13 KBJo Fitzgerald
#51 2757007-51.patch3.8 KBJo Fitzgerald
#40 2757007-40.patch10.62 KBclaudiu.cristea
#40 interdiff-to-30.txt543 bytesclaudiu.cristea
#33 interdiff.txt3.95 KBSam152
#33 2757007-33.patch11.03 KBSam152
#31 interdiff.txt543 bytesclaudiu.cristea
#31 2757007-31.patch10.62 KBclaudiu.cristea
#30 interdiff.txt1.07 KBclaudiu.cristea
#30 2757007-30.patch10.56 KBclaudiu.cristea
#28 interdiff.txt5.19 KBclaudiu.cristea
#28 2757007-28.patch10.65 KBclaudiu.cristea
#17 interdiff.txt11.84 KBclaudiu.cristea
#17 2757007-17.patch10.58 KBclaudiu.cristea
#13 interdiff.txt1.76 KBclaudiu.cristea
#13 2757007-13.patch19.86 KBclaudiu.cristea
#9 interdiff.txt2.11 KBclaudiu.cristea
#9 2757007-9.patch20.05 KBclaudiu.cristea
#8 interdiff.txt6.64 KBclaudiu.cristea
#8 2757007-8.patch19.66 KBclaudiu.cristea
#4 interdiff.txt3.03 KBclaudiu.cristea
#4 2757007-4.patch13.29 KBclaudiu.cristea
#2 convert_all_book_web-2757007-2.patch10.81 KBjibran
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jibran created an issue. See original summary.

jibran’s picture

Status: Active » Needs review
FileSize
10.81 KB

Status: Needs review » Needs work

The last submitted patch, 2: convert_all_book_web-2757007-2.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
13.29 KB
3.03 KB

I fixed all tests except one that is caused by #2767655: Allow WebAssert to inspect also hidden fields.

We should postpone this on that.

Status: Needs review » Needs work

The last submitted patch, 4: 2757007-4.patch, failed testing.

jibran’s picture

@claudiu.cristea++ thanks mate for picking this up.

claudiu.cristea’s picture

@jibran thanks. We have a green patch in #2767655: Allow WebAssert to inspect also hidden fields that could unblock this.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.66 KB
6.64 KB

Bypassed #2767655: Allow WebAssert to inspect also hidden fields by moving BookTest::testBookOrdering() into a new BookJavascriptTest (JavascriptTestBase) test.

claudiu.cristea’s picture

FileSize
20.05 KB
2.11 KB

Here we go. This is ready for review.

jibran’s picture

This looks perfect to me now. I think #8 is the correct way of handling it. That test was indeed related to JS and a JS specific test makes sense here.

+++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
@@ -0,0 +1,137 @@
+    // @todo: Investigate why if trying the reverse (2nd over 1st), the 2nd is
+    //   indented instead of moved upward. This might be a bug in tabledrag.js.

Do we want to investigate this here?

claudiu.cristea’s picture

@jibran, I would not block the conversion of test on this. I will open a new issue for that and try to reproduce the error in a dedicated test.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -255,7 +255,7 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
-      $this->assertPattern($this->generateOutlinePattern($nodes), format_string('Node @number outline confirmed.', array('@number' => $number)));
+      $this->assertPattern($this->generateOutlinePattern($nodes));

messages should not be removed with this patch, we can do that when we replace the deprecated assertPattern() method calls with WebAssert invocations.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
19.86 KB
1.76 KB

OK for assertPattern() but the others have lost the message parameter in method signature. It's clear enough for review, now.

klausi’s picture

Status: Needs review » Needs work

Whoa, a javascript test! Yippee!

  1. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,137 @@
    +    // Give javascript some time to manipulate the DOM.
    +    $this->getSession()->wait(500);
    

    The second parameter for wait() is missing. Don't simply wait here; pass a javascript snippet condition until something is visible or whatever. See https://knpuniversity.com/screencast/behat/javascript-waiting for an example. And the max wait time should be longer, for example 5000 (5 seconds).

  2. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,137 @@
    +    // Toggle row weight selects as visible.
    +    $page->findButton(t('Show row weights'))->click();
    

    Never call t() in tests. Directly assert what should be there, we are not testing the Drupal translation system here. Also elsewhere.

  3. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,137 @@
    +    $this->assertTrue($page1->book['weight'] > $page2->book['weight']);
    

    use assertGreaterThan() instead.

  4. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,137 @@
    +    $this->assertTrue($pos2 > $pos1, "'$first' precedes '$second'");
    

    assertGreaterThan()

I still think all the changes which just remove the messages should not be done because they are out of scope for this issue, but I don't care too much :)

jibran’s picture

Thank you @klausi for the review.

messages should not be removed with this patch

-1 to that. Let's not run ourselves to the ground. This is a test code we can keep on changing it all day long and it won't be done. This change is not impacting the testing scenario so let's leave it as is. I agree we shouldn't convert it but reverting it will be just like wasting time. It was my fault that I spent time on removing that additional param. Let's not waste any further time. Thank you.

claudiu.cristea’s picture

I added a new issue and a test for the drag'n'drop issue mentioned in the @todo: #2769825: Potential bug? Draggable table test.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
10.58 KB
11.84 KB

@klausi, thank you for review. I reverted also the messages.

#14.1, 3, 4: Fixed. #14.2: I disagree. Even this is not a translation test we use t() when checking for translated elements on page. I agree it will give the same result. See https://www.drupal.org/simpletest-tutorial-drupal7 section "When to use t() in simpletests" (it's for simpletest but it's the same).

Status: Needs review » Needs work

The last submitted patch, 17: 2757007-17.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

The CI says "ERROR: Step ?Publish JUnit test result report? failed: no workspace for default #181961"

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Are we actually still allowed to move along with this issues. @claudiu.cristea do you know something about it?

klausi’s picture

No, this issue is more or less postponed until we can shake out the random fails in #2776269: High random fail rate in BTB forum tests on Postgres (especially, but not only, with PHP7). In the meantime we should not commit more browser test conversions, because there is a high risk to reintroduce the random fails and give core committers headaches.

claudiu.cristea’s picture

@dawehner, no, I'm totally confused about the test conversion policy. I didn't followed the discussion. But this is a case that cannot be converted with the bulk conversion issue because has a test that need to be converted into JavascriptTestBase.

dawehner’s picture

IMHO it would be still valuable to continue ...

claudiu.cristea’s picture

@dawehner it needs review & RTBC. Is done IMO

Status: Needs review » Needs work

The last submitted patch, 17: 2757007-17.patch, failed testing.

klausi’s picture

Looks almost ready, just some minor point that should be fixed:

  1. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +  /**
    +   * Modules to install.
    +   *
    +   * @var array
    +   */
    

    this can be just be {@inheritdoc}

  2. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +    $session = $this->getSession();
    +    $page = $session->getPage();
    

    $session is not used for anything later, so this can be one line.

  3. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +    // Check that '2nd page' row is heavier than '1st page' row.
    +    $this->assertTrue($weight_select2->getValue() > $weight_select1->getValue());
    

    should be assertGreaterThan()

  4. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +    // Check that '1st page' precedes the '2nd page'.
    +    $this->assertFirstPrecedesSecond('1st page', '2nd');
    

    shouldn't the second parameter be "2nd page"?

  5. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +    // Check that '1st page' row became heavier than '1st page' row.
    +    $this->assertTrue($weight_select1->getValue() > $weight_select2->getValue());
    

    Comment does not make sense, 2 times "1st page"?

  6. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
    @@ -0,0 +1,138 @@
    +    // Toggle row weight selects back to hidden.
    +    $page->findButton(t('Hide row weights'))->click();
    

    https://www.drupal.org/node/2783189#t has now a section to never use t(), I'm also going to update the Drupal 7 variant to never use t(). We should not add unnecessary t() calls to new test code that we write.

claudiu.cristea’s picture

@klausi,

#27.1:

this can be just be {@inheritdoc}

No, unfortunately it cannot be yet. We need #2801883: Add an empty $modules property in BrowserTestBase first (Please review also that one).

#27.2-6: Fixed.

I also made assertFirstPrecedesSecond() more generic in the idea that at some point we can move it in BTB. At least I had in some projects cases where I need to make such assertions, with 2 or more than 2 elements.

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

FileSize
10.56 KB
1.07 KB

No, using FormattableMarkup is wrong with $this->assertSession()->pageTextContains() as it turned out in #2773733: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).

claudiu.cristea’s picture

FileSize
10.62 KB
543 bytes

OK, now that #2801883: Add an empty $modules property in BrowserTestBase, I've fixed also the docs for $modules. This is ready, IMO.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

To be honest, all those tests needs some adaption, so I guess its kinda fine to have them. We will not be able to fix those my moving code around.

+++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
@@ -0,0 +1,139 @@
+    $dragged = $this->xpath("//tr[@data-drupal-selector='edit-table-book-admin-{$page1->id()}']//a[@class='tabledrag-handle']")[0];
+    $target = $this->xpath("//tr[@data-drupal-selector='edit-table-book-admin-{$page2->id()}']//a[@class='tabledrag-handle']")[0];
+    $dragged->dragTo($target);

This is just super nice, that it works

Sam152’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.03 KB
3.95 KB

assertOrderInPage seems like a really handy helper method. We should add it to WebAssert so everyone can use it.

claudiu.cristea’s picture

No sorry. That should be a followup because it needs also tests and it should be separate. I think a followup is more acceptable than hiding it here.

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

As you like. Back to RTBC for #31 then.

Sam152’s picture

Is there any precedent for testing our test methods like this? I can't seem to find any. If it's acceptable for a book test, I'm not sure why it wouldn't be acceptable to be used elsewhere.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2757007-33.patch, failed testing.

claudiu.cristea’s picture

Issue tags: +Needs reroll
claudiu.cristea’s picture

@Sam152, I agree with you that the assertion could be useful in other places. But this is not in the scope of this issue. I'm not very sure that needs testing, though there are assertion methods that we are testing. @dawehner and @klausi are leading the PHPUnit initiative, maybe you should suggest them and if everybody agrees let's create a separate issue and fix there.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
543 bytes
10.62 KB

Reposting the patch from #31.

Sam152’s picture

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

#31 was already RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2757007-40.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
claudiu.cristea’s picture

It was a random bot failure. Back to RTBC as per #31.

klausi’s picture

Looks good, can we get the new javascript test as a separate child issue of #2809161: Convert Javascript/AJAX testing to use JavascriptTestBase? We can do that change immediately, the rest of book module will probably have to wait until we do the big conversion patch. See #2807237: PHPUnit initiative.

claudiu.cristea’s picture

@klausi, opened #2820079: Convert \Drupal\book\Tests\BookTest::testBookOrdering() to a JavascriptTestBase test. It's ready for RTBC. Comparing to this I only added a @todo in BookJavascriptTest::assertOrderInPage()

dawehner’s picture

I'm wondering whether with #2809181: Provide forward compatibility layer for BrowserTestBase::xpath we could make this issue be redundant.

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.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
3.8 KB

Re-roll.

klausi’s picture

Status: Needs review » Needs work

Looks like you forgot to move the files. The core/modules/book/src/Tests directory should be empty after this patch.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.13 KB

Ooops, let's try that re-roll again...

The last submitted patch, 51: 2757007-51.patch, failed testing.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -690,7 +690,7 @@ public function testAdminBookNodeListing() {
-    $this->assertEqual((string) $elements[0], 'View', 'View link is found from the list.');
+    $this->assertEqual((string) $elements[0]->getText(), 'View', 'View link is found from the list.');

the (string) cast is not needed anymore because getText() will already return a string.

Looks like your forgot to convert core/modules/book/src/Tests/BookInstallTest.php?

Then there is Views/BookRelationshipTest.php but that is again blocked on #2747167: Convert first part of web tests of views_ui and can be skipped for now.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
6.9 KB
375 bytes
tameeshb’s picture

FileSize
4.12 KB
657 bytes

Sorry, uploaded wrong patch by mistake.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

Note: I can RTBC this because the code I worked on has been moved to #2820079: Convert \Drupal\book\Tests\BookTest::testBookOrdering() to a JavascriptTestBase test.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#55 Looks like your forgot to convert core/modules/book/src/Tests/BookInstallTest.php?

Has not been addressed.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
75.65 KB
2.61 KB

Here is the patch converting BookInstallTest.php. I don't understand why my patch is this big :(

Status: Needs review » Needs work

The last submitted patch, 60: 2757007-60.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
5.41 KB

Converted "convert core/modules/book/src/Tests/BookInstallTest.php"

tameeshb’s picture

FileSize
546 bytes
4.92 KB

Redone after staging and commiting...

tameeshb’s picture

himanshu-dixit’s picture

@tameeshb I don't understand what extra you apart from #60.
UPDATE: OK, i got it. But you could have made an interdiff against #60. It would have been easy.

tameeshb’s picture

@himanshu-dixit interdiff against #60 wouldnt have made sense.
You are asking me to interdiff 75.65 KB Vs 4.92 KB!

alexpott’s picture

@himanshu-dixit - you need to configure git to detect moves. See https://www.drupal.org/documentation/git/configure - specifically the bit about Ooptimising diffs for renamed and copied files.

claudiu.cristea’s picture

@himanshu-dixit, you'll need to use the -C option when creating the patch with diff:

$ git diff -C

or you can configure Git to do this automatically. In ~/.gitconfig add:

[diff]
  renames = copies
claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

BookInstallTest is converted too, now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 63: 2757007-63.patch, failed testing.

tameeshb’s picture

Status: Needs work » Reviewed & tested by the community
tameeshb’s picture

There's something wrong with the testbot, I saw the same problem in another issue.
Update: I came across the new coding standard about using only [] for array declarations and depreciating array().
Uploaded reroll.

Eric_A’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
tameeshb’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
Jo Fitzgerald’s picture

Issue tags: -Needs reroll

Removed "Needs reroll" tag.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 2c0c8c1 to 8.4.x and 610ea72 to 8.3.x. Thanks!

  • alexpott committed 2c0c8c1 on 8.4.x
    Issue #2757007 by claudiu.cristea, tameeshb, Jo Fitzgerald, Sam152,...

  • alexpott committed 610ea72 on 8.3.x
    Issue #2757007 by claudiu.cristea, tameeshb, Jo Fitzgerald, Sam152,...

Status: Fixed » Closed (fixed)

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

himanshu-dixit’s picture