Convert all book web tests to BrowserTestBase

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

We should postpone this on that.

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

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

Here we go. This is ready for review.

@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.

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

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

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 :)

I added a new issue and a test for the drag'n'drop issue mentioned in the @todo: #2769825: Cannot swap tabledrag rows by dragging the lower over the upper row in tests.

@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).

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

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

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.

@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.

IMHO it would be still valuable to continue ...

@dawehner it needs review & RTBC. Is done IMO

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.

@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.

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).

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

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

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

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.

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

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.

@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.

Reposting the patch from #31.

Sure, sounds fine to me. Opened #2817657: Add methods to assert that a sequence of strings appears on the page in a given order as a follow up.

#31 was already RTBC.

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

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.

@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()

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

Needs a reroll.

Re-roll.

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

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

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

Sorry, uploaded wrong patch by mistake.

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.

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

Has not been addressed.

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

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

Redone after staging and commiting...

@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.

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

@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.

@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

BookInstallTest is converted too, now.

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.

Needs reroll now that #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase is in.

Removed "Needs reroll" tag.

Looks good, thanks!

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