Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watchdog.create() call test case silently failing #6042

Closed
mlewand opened this issue Jan 3, 2020 · 5 comments · Fixed by ckeditor/ckeditor5-watchdog#33
Closed

Watchdog.create() call test case silently failing #6042

mlewand opened this issue Jan 3, 2020 · 5 comments · Fixed by ckeditor/ckeditor5-watchdog#33
Assignees
Labels
package:watchdog type:bug This issue reports a buggy (incorrect) behavior.

Comments

@mlewand
Copy link
Contributor

mlewand commented Jan 3, 2020

📝 Provide detailed reproduction steps (if any)

While working on #6002 I noticed that the "should not throw an error when the destructor is not defined" unit case is causing DOM leaks.

Upon closer look it turned out that the promise returned by watchdog.create() is silently rejected (rejection caused by unhandled exception).

Two things here:

  1. First this means that this test doesn't pass, as this call per unit test should not throw exception. If you adjust the test in a way that it waits for the promise resolution then it fails.
  2. Why isn't this reported by Karma in a first place? I can see it reported when running Karma in debugging mode, but it is not reported to the console and it went unnoticed for a longer time.

📃 Other details

This issue might be related to #5897.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. package:watchdog labels Jan 3, 2020
@mlewand
Copy link
Contributor Author

mlewand commented Jan 3, 2020

In this case we should temporarily remove the test until this is resolved, so there's no leak affecting other tests. The main problem now is that it triggers a promise, and doesn't wait for it, so the leak kicks in 1 / 2 tests later (as mocha is not aware that something is till happening so it starts next test cases).

@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 3, 2020

Yep, I can confirm it, that test wait for the editor.create() to finish.

@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 3, 2020

Why isn't this reported by Karma in a first place? I can see it reported when running Karma in debugging mode, but it is not reported to the console and it went unnoticed for a longer time.

Karma (or rather mocha) mutes all promise rejections between tests unfortunately.

mlewand added a commit to ckeditor/ckeditor5-watchdog that referenced this issue Jan 3, 2020
@oleq
Copy link
Member

oleq commented Jan 7, 2020

Bonus: If you add a test after this last one https://github.com/ckeditor/ckeditor5-watchdog/blob/9b6ace1ab5050355f49f264c0231b68351d495c4/tests/watchdog.js#L1033, it will never be executed. I tried adding one with just debugger call to see the state of DOM and it never ran.

diff --git a/tests/watchdog.js b/tests/watchdog.js
index c69f21b..620df4a 100644
--- a/tests/watchdog.js
+++ b/tests/watchdog.js
@@ -1087,6 +1087,14 @@ describe( 'Watchdog', () => {
                                     } );
                   } );
          } );
+
+         describe( 'cleanup category', () => {
+                  it( 'cleanup test ', () => {
+                           expect( true ).to.be.true;
+
+                           debugger;
+                  } );
+         } );
 } );

 function throwCKEditorError( name, context ) {

@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 7, 2020

Maybe it's somehow connected with the window.onerror replacements I've made in those tests. I'll investigate it more once I've a little bit more time. Maybe there's a better solution to test error handling mechanisms.

@Reinmar Reinmar added this to the backlog milestone Jan 14, 2020
scofalik added a commit to ckeditor/ckeditor5-watchdog that referenced this issue Feb 10, 2020
Feature: Introduced `ContextWatchdog` which is a watchdog for `Context`. Closes ckeditor/ckeditor5#6079. Closes ckeditor/ckeditor5#6042. Closes ckeditor/ckeditor5#4696.

BREAKING CHANGE: The `Watchdog` class was renamed to `EditorWatchdog` and is available in `src/editorwatchdog.js`.
BREAKING CHANGE: The `EditorWatchdog.for()` method was removed in favor of the constructor.
BREAKING CHANGE: The `EditorWatchdog#constructor()` API changed, now the `EditorWatchdog` accepts the editor class as the first argument and the watchdog configuration as the second argument. The `EditorWatchdog` editor creator now defaults to `( sourceElementOrData, config ) => Editor.create( sourceElementOrData, config )`.
@Reinmar Reinmar modified the milestones: backlog, iteration 29 Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:watchdog type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants