Some JavaScript errors are not recorded during test runs

Looks like this fix uncovered a few already failing JS tests.

This should leave one failing JS test I believe.

Weird interdiff probably due to #8 being a diff.

This should leave one failing JS test I hope.

Well...let's say I replaced some errors with some other errors...

I'm afraid the last remaining JS test failure needs a bigger JS Brain than mine.
No clue what's going on there.

Got it, should be no test failures now.

Tentatively raising to Major, since there are quite a few problems with CKEditor5 JS, including a missing JS-dependency, which could potentially has nasty effects.

Thanks @znerol for the valid remark.

I don't think we need the $this->failOnJavaScriptErrors();, since tests will fail by default on those (at least after your excellent sleuthing in the related issue as well as in this one).

Also added some comments that might help people with a NotSoBigJavaScriptBrain (like me) understand what is being tested.
At the very least I needed some time to grok what was going on.

Right...
Let's go full mental and a patch as well to the same interdiff twice uploaded...
_facepalms_

When we get to the committing phase we need the whole *.es6.js-jig, besides not backporting the JS-fail tests on 9.x, since #3293090: Fail JavaScript tests on JavaScript errors never landed over there.

Meh, after some more local testing it turns out we _do_ need $this->failOnJavaScriptErrors();, at least sort of.

Also, it looks like changing failOnJavaScriptErrors halfway during a test doesn't work, so I've split up the current test in two tests.

Fixing typo.

Tested this out by applying this the patch in #27 and the test only patch from [#3305706#comment-14859013] and confirmed I got the error that was expected in [#3305706#comment-14858943]

This is great! My feedback should be fairly simple to take care of, so I imagine this can be back to RTBC soon. Feel free to ping when that's the case.

1. +++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
   @@ -110,6 +110,7 @@ internal.admin:
   +    - core/drupal.announce
   

   👍 I spotted this problem earlier today in an unrelated issue. I support fixing it here since it is in a context where test coverage is introduced that backs up the need to do this.

2. +++ b/core/modules/ckeditor5/js/ckeditor5.admin.js
   @@ -652,17 +652,19 @@
   +        if (form !== null) {
   

   Null is falsy in JavaScript so this can be if(form) and will potentially catch other invalid use cases.

3. +++ b/core/modules/ckeditor5/js/ckeditor5.admin.js
   @@ -714,7 +716,9 @@
   +            if (activeTabLink !== null) {
   

   Similarly, this can just be if (activeTabLink)

4. +++ b/core/modules/filter/filter.filter_html.admin.js
   @@ -114,12 +114,14 @@
   +        if (that.autoTags !== null) {
   

   You know what I'm gonna say 🙂

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormValuesTest.php
   @@ -34,7 +34,6 @@ protected function setUp(): void {
   -
   

   Nit: lets leave the line as-is since it's unrelated to the issue scope.

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptErrorsTest.php
   @@ -39,6 +39,21 @@ public function testJavascriptErrors(): void {
   +  public function testJavascriptErrorsAsync(): void {
   

   It's kinda evident what this is doing, but lets add the usual docblock with a description

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptErrorsTest.php
   @@ -39,6 +39,21 @@ public function testJavascriptErrors(): void {
   +    // detected.
   

   This can all fit on a single line.

Only one I wasn't sure about was #29.1

So changing if (that.autoTags !== null) {
caused a test fail? That doesn't seem right?

Actually is that the random ckeditor5 error that pops up everywhere?

Looked at maybe 100 review tickets today think I can't read anymore.

Ordered a re-test and it was indeed a random fail.

Back to NR thanks for double checking!

1. +++ b/core/modules/ckeditor5/js/ckeditor5.admin.js
   @@ -652,17 +652,19 @@
   +        if (form) {
   

   What do you think about using an early return here? IMHO this is better for the complexity, something like:

   if (!form) {
     return;
   }
   // Get the current stored UI state as an object.
   const currentStates = form.hasAttribute('data-drupal-ui-state')
     ? JSON.parse(form.getAttribute('data-drupal-ui-state'))
     : {};
   
   // Store the updated UI state object as an object literal in the parent
   // form's 'data-drupal-ui-state' attribute.
   form.setAttribute(
     'data-drupal-ui-state',
     JSON.stringify({ ...currentStates, ...states }),
   );
   

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/FormValuesTest.php
   @@ -74,10 +85,11 @@ public function testSimpleAjaxFormValue() {
   +      // Remove error.log so we have a clean slate for the next request.
   

   Just a nitpick here, it seems a comma is missing here, IMO this should be:

   "Remove error.log, so we have a clean slate for the next request."

I'll leave as NR since it's just some nitpicks the code looks goods and if you think that we should address the nitpicks I've mentioned here, feel free to update the patch and I can review it again.

I randomly found this issue mentioned on slack.
Unfortunately only after I had spent some time on learning from scratch how to do what this issue is addressing.
But I am very happy to have found this, because apart from the name of the key and the conditional the code I am using to test this on a client project with DTT looks the same.
The nit picks from #36 above are valid, however, to me 1) may be true, but we have the same construct with if(something){ something.stuff;} also in other places of the patch and it makes sense to me also the way the patch describes it. I have no preference of one or the other here. So I set it to RTBC and let the committer decide. The same goes for 2) here a committer may just add the comma. I know good language is something we should strive for, but I would not have spotted the missing comma and I was about to RTBC this issue from my phone on the weekend.

As much as I hate to switch this back, #36.1 and #36.2 are good suggestions that should probably happen.

It would also be fine to create followup issues for those small changes and switch this back to RTBC.

As much as I hate to switch this back

Sorry, that wasn't my intention :( , I think I can help by just addressing #36 here, I'm not going to move this back to RTBC because I'm addressing my own review here.

I was fine with it before, I am ok with it now too :D

@murilohp zero objection to anything you did here 🙂. It was good feedback and those kind of things absolutely help the maintainability of the code.

@bircher thanks for being at the ready to RTBC again, this really helps momentum

I committed this to 10.1.x. I'm not backporting as this could result in suprise test failures for modules/themes with previously uncaught JS errors. While it's good these errors are now being caught, having them appear in a patch release could be risky + disruptive.