Convert \Drupal\book\Tests\BookTest::testBookOrdering() to a JavascriptTestBase test

It is super nice to expand the test coverage here!

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

Super nice!

Should we document it better what is it testing? Or split it into few test cases.

1. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -0,0 +1,141 @@
   +  public function testBookOrdering() {
   

   Reading the title, I just expected this is testing re-order with dragging

2. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -0,0 +1,141 @@
   +    $page->findButton('Show row weights')->click();
   

   Until I read Line 8x, I don't know it's also tested "Show Weight" feature.

Missing?
- test Parent-Child

DEL

Everything is tested in the UI. The backend test is only an extra precaution.

Thanks. I just read the code again and see it :)

Yes, that is not exactly in the scope. It's a bonus :)

Will another test case for it? If so, should we remove it in the current test? Better Performance :)

+++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
@@ -0,0 +1,141 @@
+    $page->findButton('Hide row weights')->click();
...
+    $this->submitForm([], 'Save book pages');

Umm. It looks right.. and not so right.

If we clicked the weight column before saving. How can we tell it's changed by drag-n-drop or changed by "Weight" column JS (maybe there's a `change` event from "Weight" click triggered re-ordering)

Thanks.

I'm not reordering by weight selector.

I understand it. But any other actions could be the side-effect to the test. Although we know that no changes from the "Show Weight" click in the current code, refactoring code could change the behavior. If we re-run all these testing with Human on refactoring, then unit test is useless.

Mink able to execute JS. But I think it's not necessary in this test case. An human just drag it and click save, and then see correct ordering.

EDITED:

Although we know that no changes from the "Show Weight" click in the current code,

In fact, until I actually reading the JS code carefully. I don't know if it's true or not. My eyes looking at UI, it's not. But it could be work in a way I don't expect.

It's wrong in Browser Acceptance test. Let me try to explain it.

Test is guaranteed the feature works as expected, no matter what we changed the code in other places.

Do we agree with it? If not, I think too much :(

Above test catches the drag-n-drop ordering.

Now, we refactoring the "Weight Column" to RE-SYNC the value to more readability (e.g. From 1 to 10 instead of -15, -9 -6) on clicking.

We'd sure that ABOVE TEST is covered the drag-n-drop. We run the Patch, it's GREEN. It's passed! It committed!

However, we introduced a new bug on refactoring. It broke the drag-n-drop to SYNC THE VALUE.

In this case, above test case still passed!! Because it clicked the "Weight Column" to trigger the event fired and changing the value correctly!

Current:
DRAG -> VALUE changed by DRAG -> click the weight column -> save

Then:
I introduced a new feature to weight click and a bug to drag-n-drop.
CLICK WEIGHT COLUMN -> VALUE change by NEW Feature -> save
(We know that drag-n-drop has test covered, we won't spend time on testing with HUMAN.)

After this bad refactoring
DRAG -> (NO CHANGE TO VALUE) -> CLICK WEIGHT COLUMN -> CHANGED VALUE FROM WEIGHT -> SAVE

Is the drag-n-drop really working? No!

No extra tests but removing the extra `Show rows weight` clicking

I reviewed the patch again. It's only ONE SAVE at the end of tests. I hope I don't miss anything.

This is how above patch does:
Drag -> [Change Value] -> Click `Show rows weight` -> [Change test factors (SIDE-EFFECT)] -> Click Save

We should remove the side-effect in tests.

Best example:
Drag -> [Change Value] -> Click Save -> [Perform checking with `document.querySelectorAll` like selectors, Mink's assertions]

Good example:
Drag -> [Change Value] -> [Checking the result with `document.querySelectorAll` like selectors which will not modify the HTML] -> Click Save

** I will keep it as RTBC. But I hope the committers/maintainers give me one more chance to explain it if you disagree/don't get my point also. Don't think in PHP server-side testing with generated HTML output.

Thanks ALL

@droplet I don't get the reservations you have, I have reviewed the test and it does multiple checks to see whether the reordering was successful.

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

   It first checks to see if the dragging actually results in the items being placed in the right order in the DOM.

2. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -0,0 +1,141 @@
   +    // Check that rows weight selects are visible.
   +    $this->assertTrue($weight_select1->isVisible());
   +    $this->assertTrue($weight_select2->isVisible());
   +
   +    // Check that '1st page' row became heavier than '2nd page' row.
   +    $this->assertGreaterThan($weight_select2->getValue(), $weight_select1->getValue());
   

   Then it checks that the weights have been correctly updated in the DOM. The actual numbers don't matter, but the dragged item should now have a higher weight.

3. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -0,0 +1,141 @@
   +    $this->submitForm([], 'Save book pages');
   +    $this->assertSession()->pageTextContains(new FormattableMarkup('Updated book @book.', ['@book' => $book->getTitle()]));
   +
   +    // Check again that '2nd page' is on top after form submit.
   +    $this->assertOrderInPage(['2nd page', '1st page']);
   

   It submits the form and checks again that the correct order is retained, this already proves that the weights have been correctly updated and saved.

4. +++ b/core/modules/book/tests/src/FunctionalJavascript/BookJavascriptTest.php
   @@ -0,0 +1,141 @@
   +    // Check that page reordering was done in the backend.
   +    $page1 = Node::load($page1->id());
   +    $page2 = Node::load($page2->id());
   +    $this->assertGreaterThan($page2->book['weight'], $page1->book['weight']);
   

   Then it does an additional check to see that the weights that are saved in the database are in the correct order. This is not strictly needed, but it *REALLY* proves it now.

For me this looks really sufficient for the current implementation.

If we would implement the weight re-syncing example that you give, then that should have an additional javascript test that proves that the weight re-syncing does not affect previous unsaved reordering by drag-and-drop.

@droplet
This patch already improves the status a lot. We could though, as you know, always iterate on top of it. Creating issues is easy.

Then it checks that the weights have been correctly updated in the DOM. The actual numbers don't matter, but the dragged item should now have a higher weight.

You skipped the most important point:
It can check the DOM with test assertions (result, the DOM is snapshoted). But it should not modify the DOM and then check it.

What it should be tested is a single action: Drag => GREEN result
Above test is testing a combined action:Drag + Weight Click => GREEN result

It's lucky! The DOM changes do not make any differences to test assertions above.

@pfrenssen comment about re-syncing example isn't true all time. Any new test on weight is testing weight features or a drag + weight combination. But tests will not test drag-n-drop alone again.

@dawehner,
OK.

Somehow, I agreed my suggestions is silly. We almost never make an extra change to `Weight` link in CORE.

Thanks All. Have A Nice Weekend!

@droplet

If I understand correctly:

The test now tests the following:

1. Use drag-n-drop to change the order
2. Validate the UI (checks if reordering works)
3. Enable the weight select boxes
4. Validate the changed values in the weight selects
5. Hide the weight fields and check they've been hidden
6. Submit the page
7. Validate the order on the server

The problem is that the server check relies on the drag-n-drop AND the switch to the manual input. Drag-n-drop is not checked isolated from the weight fields.

I changed the patch to submit the page directly after the drag-n-drop action and then validate on the server. After that, I use the weight fields directly and submit the page. Again I check the values on the server. This way the drag-n-drop is tested separately from the weight fields.

Other small change: assertTrue / assertFalse is deprecated in 9.x (should a patch be posted to mark each method deprecated to make the IDE pick it up?). I changed them to assertEquals.

@michielnugter,

Step 1-7 in your comment #22 is how above patch is tested.

But with this test scope, this is only test drag-n-drop. Correct order should be:
1. Use drag-n-drop to change the order
2. Validate the UI (checks if reordering works)
3. Validate the Value in Hidden Weight field (without interactive click, use getHTML, test assertions)

4. Submit the page
5. Validate the order on the server

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

Point #3 is important. "An action interacts with UI to validate the result" vs "Validate with snapshotted result (->getHTML)"

Ideally, a strict test should validate with getHTML before interact with UI `->click` again. (Good test won't make assumptions)

I read the full #22 patch again. Yes. it's improved.

It performed `submitForm & validate` the order before clicking `weight` link.

Didn't know about the messages, I removed them. Also put quotes around the 1st page text.

As for assertTrue, the method is defined in the 'AssertLegacyTrait' trait. This was deprecated in #2304461.

@droplet: The hidden field value is now tested implicitly by posting it to the server and validating in there, is that enough?

@michielnugter #26,

That's fine. I was read the interdiff and doesn't realize that in my first response. Thanks!

Now the assertTrue extend from:
https://github.com/drupal/drupal/blob/b33af7a964ee34171e3328c3276d63551a...

When it removed, it will be used function from PHPUnit directly (if I read the source correctly. JavascriptTestBase doesn't involve TestBase)

(I don't know the preferred method anyway)

@claudiu.cristea

JavascriptTestBase doesn't implement the assertTrue but the PHPUnit_Framework_Assert class does define a (static?) assertTrue, this class is extended via-via. As to why the Trait is still included is not clear to me then.

Sorry about the mixup, I reverted the method calls back.

Edit:
... Now it's clear:

$this->assertTrue() is deprecated, it actually does call the method in AssertLegacyTrait. The documentation states we should use the following:

self::assertTrue()

Which conforms to the static definition of the assertTrue method in PHPUnit_Framework_Assert. It seems the recommended way is self:: for every assert?

@claudiu.cristea

JavascriptTestBase extends via-via PHPUnit_Framework_Assert method. This is the only ancestor that defines the assertTrue method, statically.

So without the deprecated Trait, the method is only callable statically. $this->assertTrue() is really deprecated in favor of self::assertTrue().

As the $this->assertTrue() style is still used all over the place, even in the BrowserTestBase class I don't think it should stop this patch. I however do think we should try to avoid deprecated methods when writing new patches.

So without the deprecated Trait, the method is only callable statically.

Well, this is kinda wrong ... static means it can be also called statically, but it is still a normal method.

. $this->assertTrue() is really deprecated in favor of self::assertTrue().

That change for itself should be discussed in its issue, please create one. Note: Sebastian Bergmann though wrote an response there: https://github.com/sebastianbergmann/phpunit/issues/1914#issuecomment-14...

@dawehner

Thanks for explaining. It feels really weird to not call a static method statically but if it's the way to go I'll follow.

Sorry about the confusion and setting the issue back unnecessarily.

I think the test is a bit heavy on testing tabledrag functionality rather than the book ordering but we can refactor that once a tabledrag test exists. Nice to see proper JS testing happening.

Committed and pushed 7522a3d to 8.3.x and dc329cc to 8.2.x. Thanks!

Committed to 8.2.x to keep test coverage the same.

Sorry about the confusion and setting the issue back unnecessarily.

No worries. Its always good to talk about stuff.