Problem/Motivation

Not failing JavaScript test for JavaScript errors is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3221100

Steps to reproduce

Proposed resolution

Fail a JavaScript test when a JS error is encountered.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Tests will now fail on JavaScript errors. Previously, any JavaScript error that occurs that did not interfere with the functionality of the test was ignored. Now those errors are surfaced to the test runner. See the change record for details.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
2.3 KB

So we can convert the deprecation to a failure, but as the failure happens in tearDown() I don't see how to catch and expect this in a test.

Status: Needs review » Needs work

The last submitted patch, 2: 3293090-2.patch, failed testing. View results

longwave’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

As we are explicitly testing the tearDown() method I think it is OK to call that directly from the test. We also then need to clear the error log once we have retrieved it, or the test will fail for real.

longwave’s picture

mondrake’s picture

Would it make sense to extract part of the code in tearDown to a protected method of its own, and call that method from tearDown and from the test instead of calling tearDown from within the test (which OK, seems to work here, but who knows in the future)?

longwave’s picture

#6 also means we can move the reset that is only required for the test into the test itself.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Nice.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
@@ -115,18 +115,25 @@ protected function tearDown(): void {
+  /**
+   * Triggers a test failure if a JavaScript error was encountered.
+   *
+   * @throws \PHPUnit\Framework\AssertionFailedError
+   */
+  protected function failOnJavaScriptErrors(): void {
+    if ($this->failOnJavascriptConsoleErrors) {
+      $errors = $this->getSession()->evaluateScript("JSON.parse(sessionStorage.getItem('js_testing_log_test.errors') || JSON.stringify([]))");
+      if (!empty($errors)) {
+        $this->fail(implode("\n", $errors));
+      }
+    }
+  }

I had a look at whether we got use the @after annotation but unfortunately that does not work. tearDown is called first.

I think conceptually the if ($this->failOnJavascriptConsoleErrors) { should wrap the call to this function in tearDown() - that way if someone does calls this function in a test it will always test the js_testing_log_test.errors regardless of that setting - so you can set to FALSE for the main check but then call this yourself and it'll work as expected.

longwave’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
2.4 KB

#9 is a good suggestion, thanks. Here's a further modified approach that uses assertPostConditions(), which I think is intended for this sort of check, and which also means we don't need to reset at all in the test.

longwave’s picture

Fixing a missed comment.

alexpott’s picture

Maybe it is time to start using PHPUnit's annotations. We can use @postCondition here.

So in the JavascriptErrorsTest we do:

  /**
   * @postCondition
   */
  public function clearErrorLog() {
    // Clear the JavaScript error log to prevent this test failing for real.
    $this->getSession()->executeScript("sessionStorage.removeItem('js_testing_log_test.errors')");
  }

And then in WebDriverTestBase we remove assertPostConditions and do

  /**
   * Triggers a test failure if a JavaScript error was encountered.
   *
   * @throws \PHPUnit\Framework\AssertionFailedError
   * @postCondition
   */
  protected function failOnJavaScriptErrors(): void {
    if ($this->failOnJavascriptConsoleErrors) {
      $errors = $this->getSession()->evaluateScript("JSON.parse(sessionStorage.getItem('js_testing_log_test.errors') || JSON.stringify([]))");
      if (!empty($errors)) {
        $this->fail(implode("\n", $errors));
      }
    }
  }

Yes this reverts the change in #9 but the usage of annotations is neater and we have less inheritance funkyness.

Also I think the whole flag is kinda pointless if we do it this way. A test could make failOnJavaScriptErrors a no-op and be done.

longwave’s picture

What controls the order of these postcondition hooks? We need to ensure the clear one runs before the fail one in this case. I see the PR that adds @postCondition but it's not documented at https://phpunit.readthedocs.io/en/9.5/annotations.html :(

longwave’s picture

Answering my own question: it uses ReflectionClass::getMethods(), which according to docs

ReflectionClass::getMethods() sorts the methods by class (lowest in the inheritance tree first) then by the order they are defined in the class definition:

These are executed in order so @postCondition in the class itself should come first, then the parent class afterwards. (@preCondition is reversed, which also makes sense)

longwave’s picture

Implemented #12.

For now I think we should keep the flag as we have documented that that is the way to use this feature, we can consider removing it again later in the 10.x cycle I think?

longwave’s picture

We also don't need @group legacy any more for this test.

pooja saraah’s picture

Fixed failed commands on #16
Attached interdiff

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Although I don't know a lot about JS testing, the code looks very sensible and tests the new behaviour well AFAIS based on @alexpott's suggestion.

Gábor Hojtsy’s picture

  • catch committed b7d642b on 10.0.x
    Issue #3293090 by longwave, pooja saraah, mondrake, alexpott, Gábor...
  • catch committed 8836d06 on 10.1.x
    Issue #3293090 by longwave, pooja saraah, mondrake, alexpott, Gábor...
catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks good to me too including the adaptation of the existing test coverage.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

Status: Fixed » Closed (fixed)

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

phenaproxima’s picture

Status: Closed (fixed) » Needs work
Issue tags: +10.0.0 release notes, +Needs release note

I think this could use a release note, since it is a breaking change in our testing API.

bbrala’s picture

Issue summary: View changes
bbrala’s picture

Wrote a snippet and added to the draft.

phenaproxima’s picture

Status: Needs work » Fixed
Issue tags: -Needs release note

Looks good to me!

Status: Fixed » Closed (fixed)

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