Problem/Motivation

Our phpunit based functional javascript test will pass even if there are Javascript errors on the page.

While we test JS functionality sometimes a JS error will not break our tests because the core functionality still works but other JS that is the page of a particular site could be broken.

A user ran into this scenario on #3072231: Custom blocks break layout builder module - Quick Edit could not associate the rendered entity field markup

Proposed resolution

if a Javascript error is thrown to during a test fail the test.

Additionally

  1. All core tests should fail on Console errors by default. Each class should not need to set a property so that we cannot ignore console errors in core tests by forgetting this property
  2. Core tests should be able to set the property protected $failOnJavascriptConsoleErrors = FALSE; to suppress errors for 2 reasons
    1) This will allows use to commit this with known errors such as #3091878: MediaEmbedFilterConfigurationUiTest throws a Javascript Error or any others we find at the time we are able to commit this this issue. Before we commit this at any point we can't be sure how many tests will fail with this patch applied. We could be adding new tests that throw JS console errors with any commit.
    2)After this patch is committed we could have tests start to fail for a number of reasons such as, changes in the chrome driver/browser Drupalci uses or needed updates to our JS dependences. In the example of our JS dependencies these could be security updates that we need ASAP but also cause a small non-important JS console error. In such cases we should be able to commit the dependency updates and do a follow-up to fix the error.
  3. Any class that extends \Drupal\FunctionalJavascriptTests\WebDriverTestBase except core test, i.e Contrib and custom test, should have to opt in this behavior by setting protected $failOnJavascriptConsoleErrors = TRUE;. This will ensure a bunch of contrib tests will not start to fail after we commit this patch. We could consider changing this to opt out in Drupal 10
  4. Contrib/custom tests should be able to opt in a bunch of classes at once by setting protected $failOnJavascriptConsoleErrors = TRUE; in a base test class like, MyModuleTestBase

Remaining tasks

All

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

FunctionalJavascript tests now automatically check to see if any JavaScript errors have been thrown during the test's execution. In Drupal 10, this will cause the test to fail, while in Drupal 9.3, a deprecation warning is issued for tests that pass despite the presence of JavaScript errors. See https://www.drupal.org/node/3221100 for details.

CommentFileSizeAuthor
#33 3091870-32-cleanup.patch1.92 KBeffulgentsia
#9 3091870-9.patch19.2 KBtedbow
#9 interdiff-7-9.txt6.22 KBtedbow
#7 3091870-7.patch15.34 KBtedbow
#2 js-test-errors.patch16.54 KBtedbow

Issue fork drupal-3091870

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs review
StatusFileSize
new js-test-errors.patch16.54 KB

Currently I was only to figure out how to do this by wrapping Drupal.throwError() but maybe there is a better way.

I followed the example #3070521: Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used. so basically any call to Drupal.throwError() will save an error in the localStorage. Then in \Drupal\FunctionalJavascriptTests\WebDriverTestBase::tearDown() the session variable is checked and a test fail is set if any errors were thrown.

I altered drupalci.yml to only run FunctionalJavascript tests to see how many tests break.

Status: Needs review » Needs work

The last submitted patch, 2: js-test-errors.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +blocker
catch’s picture

catch
he/him
Primary language English
commented
Category: Feature request » Task
Priority: Normal » Major
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

A brief comment about tour module in Nightwatch before I forget.

tedbow: I don't think we would even need a test module. Just enable tour and goto /admin. Not sure about a Nightwatch test but we should be able to prove it with FunctionalJavascript for now.

Nightwatch uses the nightwatch test install profile, which doesn't have any modules that utilize tour so javascript wouldn't get installed. I needed to install the tour_test module as well. This probably doesn't apply to FunctionalJavaScript using minimal or standard.

I'm not sure why the test-only patch didn't fail on drupalci, but it did fail locally. It could be a race condition in page load versus javascript-readiness.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3091870-7.patch15.34 KB

@mradcliffe thanks for the context.

I think maybe we should keep this issue for just tests that extend \Drupal\FunctionalJavascriptTests\WebDriverTestBase and then have different but related test if we want to do the same thing with Nightwatch tests.

Here is reroll. I just had to remove the drupalci.yml changes

Status: Needs review » Needs work

The last submitted patch, 7: 3091870-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue summary: View changes
StatusFileSize
new interdiff-7-9.txt6.22 KB
new 3091870-9.patch19.2 KB
  1. This is a comment I meant post here back in November 2019 but posted it by mistake on a related issue I created #3091878: MediaEmbedFilterConfigurationUiTest throws a Javascript Error

    Wow only 1 test fails!

    I opened up #3091878: MediaEmbedFilterConfigurationUiTest throws a Javascript Error. If we could fix that we could do the current issue.

    Do we have to take into contrib or custom tests that extend WebDriverTestBase

    If so we check to if it is a not a core test in \Drupal\FunctionalJavascriptTests\WebDriverTestBase::tearDown() and then not fail the test unless the contrib test has opted into this behavior.

    If we could do this by 8.8.0 we could throw a deprecation error that this class will fail all tests with JS errors in 9.0.0.

    So this references the patch in #2

    The 1 fail was in MediaEmbedFilterConfigurationUiTest

  2. #7 had the same fail plus 1 deprecation I will fix now.
  3. We still need a way to make sure contrib won't all start failing and have this be an opt in for non-core test.
  4. Adding another test, JavascriptErrorsSuppressionTest, to prove we can suppress errors in core tests
  5. updating summary
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Needs work

#9.3

We still need a way to make sure contrib won't all start failing and have this be an opt in for non-core test.

NW for that

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Needs review

We still need a way to make sure contrib won't all start failing and have this be an opt in for non-core test.

I'm not sure this is something we should be concerned of. Previously, we have decided to avoid complexity with the risk of contrib tests failing. If there are JS errors in a contrib module, they should be aware of it. If they want to ignore the errors, they have the option to suppress the errors.

Status: Needs review » Needs work

The last submitted patch, 9: 3091870-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tedbow opened merge request !455

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Hiding patches now that an MR is open

bnjmnm made their first commit to this issue’s fork.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott made their first commit to this issue’s fork.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review

setting to NR

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Assuming the status should be NW for the above comment on the MR.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

@tedbow and I discussed and in D9 the errors will be surfaced as a deprecation, and only made a true failure in D10.
CR is here: https://www.drupal.org/node/3221100

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review

Setting to needs Review because I addressed MR comments. This is much simpler now.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Thanks @tedbow! I really like where this ended up.

phenaproxima made their first commit to this issue’s fork.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@lauriii didn't unRTBC this but left some nits. I confirmed that @phenaproxima fixed all the issue @lauriii brought up

So we can consider this me re-RTBCing this

effulgentsia’s picture

effulgentsia commented

Adding issue credits.

effulgentsia’s picture

effulgentsia commented

The patch looks great, but I'm concerned about the removal of the js_deprecation_log_test module and session storage variable and renaming it with js_testing_log_test. I think for specific test fixtures our BC policy doesn't need to be super strict, but this affects WebDriverTestBase and therefore all of its subclasses, so there's more chance of some contrib or custom functional test out there that accesses the module name or session storage variable directly. I recommend leaving js_deprecation_log_test alone, and just adding js_testing_log_test for the errors. And then having a follow-up issue for consolidating warnings and errors into the same test module and session storage variable, if we want to advocate for the benefits of that outweighing the BC break, or to do it via a deprecation process.

Leaving this at RTBC though because the above is just my opinion, and happy to be overruled by another committer.

alexpott’s picture

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

@effulgentsia contrib is currently clear - http://grep.xnddx.ru/search?text=js_deprecation_log_test&filename= I think adding this to the CR here is good enough. FWIW client projects I work on use this patch and it is vital to helping developers test that and Javascript isn't unexpected triggering errors.

alexpott’s picture

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

I've added the module name change to the CR - as part of doing this the following thought occurred - as this module is enabled in \Drupal\FunctionalJavascriptTests\WebDriverTestBase::installModulesFromClassProperty() there really is not much reason for contrib or custom to ever use it's name.

  • effulgentsia committed e4284d2 on 9.3.x 
    Issue #3091870 by tedbow, phenaproxima, alexpott, bnjmnm, tim.plunkett,...
effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +9.3.0 release notes
StatusFileSize
new 3091870-32-cleanup.patch1.92 KB

Thanks. I'm satisfied with #30 and #31. Pushed to 9.3.x, but I noticed that doing so left behind 2 dead JS files. Here's a patch to remove them. As this is just dead code removal, marking this issue fixed, and I'll commit this patch (or someone else can) once tests pass.

Also, tagging this for release notes mention. It would be great if someone could add a release notes snippet to the issue summary, while this is still fresh in our minds.

alexpott’s picture

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

@effulgentsia nice! +1 to #33 - I just noticed those files hanging about on client project and was wondering why they were still there :)

  • effulgentsia committed 84f1134 on 9.3.x 
    Issue #3091870 followup by effulgentsia, alexpott: Remove dead code.
effulgentsia’s picture

effulgentsia commented
Issue summary: View changes

I committed #33, and added a release note snippet to the issue summary. Please feel free to update it as needed.

What I wrote is:

FunctionalJavascript tests now automatically check to see if any JavaScript errors have been thrown during the test's execution. In Drupal 10, this will cause the test to fail, while in Drupal 9.3, a deprecation warning is issued for tests that pass despite the presence of JavaScript errors. See https://www.drupal.org/node/3221100 for details.

However, it looks to me like WebDriverTestBase::tearDown() runs:

@trigger_error("Not failing JavaScript test for JavaScript errors is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. This test had the following JavaScript errors: $all_errors. See https://www.drupal.org/node/3221100", E_USER_DEPRECATED);

regardless of whether the test otherwise passed or not. Am I reading that correctly, and if so, do we want a followup to restrict the deprecation to only when the test passed?

Status: Fixed » Closed (fixed)

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

znerol’s picture

znerol commented

Follow-up: #3331835: Some JavaScript errors are not recorded during test runs