Problem/Motivation

If you do not set a SIMPLETEST_BASE_URL environment variable and run a test that extends JavascriptTestBase, JavascriptTestBase::tearDown() will still be run even though ::setUp() throws an exception. JavascriptTestBase::tearDown(), however, assumes that $this->mink is set so it fatals in this case.

Proposed resolution

Add a check if $this->mink is set inside JavascriptTestBase::tearDown(). This is inline with e.g. BrowserTestBase::tearDown().

Remaining tasks

Review patch. (See the "no-whitespace" patch, which was rolled with -w.)

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#7 2817115-7.patch2.35 KBtstoeckler
#7 2817115-interdiff.txt1.15 KBtstoeckler
#4 2817115-4.patch2.35 KBtstoeckler
#4 interdiff-0-4.txt470 byteststoeckler
jtb-no-mink--no-whitespace--do-not-test.patch934 byteststoeckler
jtb-no-mink.patch1.9 KBtstoeckler

Comments

tstoeckler created an issue. See original summary.

mtodor’s picture

mtodor commented

I have tried this patch, it works fine and solution is ok.

It's also quite annoying issue, because exception thrown from JavascriptTestBase::tearDown "hides" real problem behind.

dawehner’s picture

dawehner
Primary language German
commented

Ah damnit, we had a similar issue for BrowserTestBase. A good review could have caught that ...

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -47,16 +47,18 @@ protected function initMink() {
+    if ($this->mink) {

I guess we should document \Drupal\Tests\BrowserTestBase::$mink to also be |null as long its not initialized.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new interdiff-0-4.txt470 bytes
new 2817115-4.patch2.35 KB

So, something like this?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Yeah, that looks great for me

cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -47,16 +47,18 @@ protected function initMink() {
+      $result = $this->getSession()->wait(5000, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');

    I don't actually know the policy on magic numbers. If anybody knows or cares, change it. 

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -47,16 +47,18 @@ protected function initMink() {
+        // missing database tables, because tear down will have removed them. Rather
+        // than allow it to fail, throw an explicit exception now explaining what

    The comment exceeds the 80 column width. 

  3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
@@ -47,16 +47,18 @@ protected function initMink() {
+        throw new \RuntimeException('Unfinished AJAX requests whilst tearing down a test');

    "whilst" is British English. As far as I know we use American English.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
StatusFileSize
new 2817115-interdiff.txt1.15 KB
new 2817115-7.patch2.35 KB

1. I tend to agree, but changing this is out of scope of this issue, this is used in a lot of other places in the same way.

2.+3. Fixed

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you @cilefen and @tstoeckler

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Is this untestable?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I think so, yes. At least I have no idea, how I could manage to create an error in mink setup just for a single test.

xjm’s picture

xjm
she/her
Primary language English
commented

Regarding #6 and #7, yeah, it is out of scope to fix those here since they only reason they are changed is just a change to indenting. This can be seen with a git diff -w. Edit: And I see the summary mentions that too. Super. :)

If we had the same bug in BTB, is there any way we refactor so that it fixes both? In the caller or something? It's a bit worrisome to patch it in both places for a silent no-op. Even more so if it's not testable.

Is this the hunk in question in BTB?

 if ($this->mink) {
    $this->mink->stopSessions();
  }

In that case it's obvious why $this->mink needs to be set; maybe in this case we could at least add an inline comment.

Not sure if my suggestions are actually helpful though, so leaving at RTBC for now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

So whilst I think the whilst to while change is out-of-scope and, whilst it might

reek of pretension in the work of a modern American writer

(emphasis mine - http://blog.oxforddictionaries.com/2016/02/while-or-whilst/), I think we're good to go here :)

I can't work out a way of testing this and this type of change ensures that people can focus on the real error so committing.

Committed and pushed aa5da27 to 8.3.x and 5d59627 to 8.2.x. Thanks!

  • alexpott committed aa5da27 on 8.3.x 
    Issue #2817115 by tstoeckler, dawehner, cilefen: JavascriptTestBase::...

  • alexpott committed 5d59627 on 8.2.x 
    Issue #2817115 by tstoeckler, dawehner, cilefen: JavascriptTestBase::...

Status: Fixed » Closed (fixed)

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