Kudos all-round for the new WebDriver based tests!

I have found a few (tiny) nitpicks:

  • assertNotNull returns void, so the if-construct that was used in some places isn't needed. If it fails => test fails.
  • pressButton takes only one argument: the locator.
  • fieldNotExists doesn't have a third argument for message and the second argument (container) is NULL by default.
  • Some typos
CommentFileSizeAuthor
#9 test_nitpicks-3103616-8.patch8.85 KBspokje
#8 interdiff_3-8.txt3.54 KBspokje
#4 test_nitpicks-3103616-3.patch9.54 KBspokje
#4 interdiff_2-3.txt1.11 KBspokje
#2 test_nitpicks-3103616-2.patch9.52 KBspokje

Comments

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new test_nitpicks-3103616-2.patch9.52 KB

Let's see if TestBot likes this.

spokje’s picture

spokje
Primary language Dutch
commented

Hmm, looks like the Assertions in the if branch of SimpleWidgetTest testSimpleValidation never ran because assertNotNullreturned void instead of TRUE.

Updated patch incoming.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new interdiff_2-3.txt1.11 KB
new test_nitpicks-3103616-3.patch9.54 KB

Assertions were indeed never triggered in the if-branches, since another assertion failed locally.

The failing one that TestBot complained about passed locally, but it looks like getting a canonical URL from a node has to be done a bit different in TestBot land.

Let's see how this goes.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review

What's good enough for TestBot, is good enough for me review.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

Thanks a lot for working through this and fixing such a lot of nits! I really like doing this in a separate commit.

  1. +++ b/tests/src/FunctionalJavascript/ComplexSimpleWidgetTest.php
@@ -104,16 +104,14 @@ class ComplexSimpleWidgetTest extends InlineEntityFormTestBase {
-        if ($this->assertNotNull($host_node->ief_complex_outer->entity, 'Outer node was created.')) {
-          $outer_node = $host_node->ief_complex_outer->entity;
-          $this->assertEquals($outer_title, $outer_node->label(), "Outer node's title looks correct.");
-          $this->assertEquals('ief_simple_single', $outer_node->bundle(), "Outer node's type looks correct.");
-          if ($this->assertNotNull($outer_node->single->entity, 'Inner node was created')) {
-            $inner_node = $outer_node->single->entity;
-            $this->assertEquals($inner_title, $inner_node->label(), "Inner node's title looks correct.");
-            $this->assertEquals('ief_test_custom', $inner_node->bundle(), "Inner node's type looks correct.");
-          }
-        }
+        $this->assertNotNull($host_node->ief_complex_outer->entity, 'Outer node was created.');
+        $outer_node = $host_node->ief_complex_outer->entity;
+        $this->assertEquals($outer_title, $outer_node->label(), "Outer node's title looks correct.");
+        $this->assertEquals('ief_simple_single', $outer_node->bundle(), "Outer node's type looks correct.");
+        $this->assertNotNull($outer_node->single->entity, 'Inner node was created');
+        $inner_node = $outer_node->single->entity;
+        $this->assertEquals($inner_title, $inner_node->label(), "Inner node's title looks correct.");
+        $this->assertEquals('ief_test_custom', $inner_node->bundle(), "Inner node's type looks correct.");
       }
    assertNotNull returns void, so the if-construct that was used in some places isn't needed. If it fails => test fails.

    Good find. Can we leave the if (isset(...)) statements in though? IMHO they make code dependencies and test results more readable.

So if we fix that, i'll go and commit it.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_3-8.txt3.54 KB

There you go.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new test_nitpicks-3103616-8.patch8.85 KB

Adding an actual patch file might come in handy... :/

oknate’s picture

oknate
Location Greater New York City Area
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • geek-merlin committed 749652c on 8.x-1.x authored by Spokje 
    Issue #3103616 by Spokje, geek-merlin, oknate: Nitpicks about the new...
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Reviewed & tested by the community » Fixed

OK, committed and pushed. Thank 'ya all!

Status: Fixed » Closed (fixed)

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