Failing MentionUI unit tests on CI

[mlewand]

Provide a description of the task

There are two failing tests on CI after merging #4619 PR:

Chrome 79.0.3945 (Linux 0.0.0) MentionUI typing integration asynchronous list with custom trigger should discard requested feed if they came out of order FAILED
	AssertError: expected show to be called once but was called twice
	    show() at BalloonPanelView.attachTo (webpack:///./packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.js?:230:8)
	    show() at BalloonPanelView.attachTo (webpack:///./packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.js?:230:8)
	    at Object.fail (node_modules/sinon/pkg/sinon.js:166:21)
	    at failAssertion (node_modules/sinon/pkg/sinon.js:125:16)
	    at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:151:13)
	    at eval (webpack:///./packages/ckeditor5-mention/tests/mentionui.js?:1145:20)
Chrome 79.0.3945 (Linux 0.0.0) MentionUI typing integration asynchronous list with custom trigger should fire requestFeed:discarded event when requested feed came out of order FAILED
	AssertError: expected show to be called once but was called twice
	    show() at BalloonPanelView.attachTo (webpack:///./packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.js?:230:8)
	    show() at BalloonPanelView.attachTo (webpack:///./packages/ckeditor5-ui/src/panel/balloon/balloonpanelview.js?:230:8)
	    at Object.fail (node_modules/sinon/pkg/sinon.js:166:21)
	    at failAssertion (node_modules/sinon/pkg/sinon.js:125:16)
	    at Object.assert.<computed> [as calledOnce] (node_modules/sinon/pkg/sinon.js:151:13)
	    at eval (webpack:///./packages/ckeditor5-mention/tests/mentionui.js?:1198:20)

Tests fail only if enough tests are ran, they won't fail if you yarn run test -f mention.

After investigation it turned out that the reason for it is that there are scorll events fired once mocha gets to execute mention plugin tests. That's because there are many, many editor leftovers after prior tests that haven't been cleaned up correctly.

This is the source for scroll events.

[Reinmar]

We had this issues in the past multiple times. We don't do a perfect job when it comes to tearing down some of the tests. This could be improved for sure. Also, we could have some automated check for whether there are some editors left in the DOM. That'd be a first good step. We would still miss virtual editors, but they don't cause such big issues.

[Reinmar]

PS. There are actually two scenarios possible:

1. You forgot to do any tear down and then the entire editor + its DOM elements are left.
2. You destroyed the editor but left its source element.

The former is easier to check. The latter is a bit more tricky, but perhaps some heuristic can be written for it too.

[jodator]

OMG - I've just got that thought that this might be a reason that our memory leaks were failing... Anyway, some automated test would be cool but the simplest way to check this might be seen after what tests this number grows (must be put in the global context):

afterEach( () => {
	console.log( 'I have this many editors:', document.querySelectorAll( '.ck-editor' ).length );
} );

After that, I've found that for instance, CKFinder adapter tests leaked 11 editors: ckeditor/ckeditor5-adapter-ckfinder#15.

[mlewand]

I can already tell that we have quite a few tests that do poor job with cleaning up the editor / DOM.

As a test I just did

Array.from( document.querySelectorAll('div.ck.ck-reset.ck-editor.ck-rounded-corners') ).map( el => el.remove() )

before the MentionUI suite and it fixes the tests.

Now we can either:

1. Patch each test that leaves the garbage, but this will be a lot of work and low ROI.
2. We can simply automate this cleanup and perform it after each test suite.

I'm more keen towards the latter as it requires much less work while mitigates the issue.

I'm also thinking about adding a check on this. We could even prevent this from happening in the second approach. We could simply add at the end extra test case where it asserts dead editor count against known number of tests. It will catch the case when someone adds new leftover.

[jodator]

Patch each test that leaves the garbage, but this will be a lot of work and low ROI.

I disagree. We still going to have poor tests and some potential problems. The only problem is time.

The job might be big but the ROI is not so easy to calculate IMO since this is a technological dept we have and never have time to properly fix it. It will hount us.

Maybe a temporary fix with cleanup + warning ("Yo, I've cleanup XX editors for you") to the trick and we can clean each package in spare time?

Also, I see a potential benefit in re-enabling memory leak tests and bring some quality of the project by fixing our tests.

[mlewand]

I haven't published all the fixes yet, there are still little below 30 unpatched cases (out of over 200).

I'm also sharing a temporary, easy way for you to detect leaks per test case.

To do that checkout i/6002 branch in the ckeditor5-dev repository (and make sure you yarn link the ckeditor5-dev-tests package as the script is added there).

After doing that just run your usual yarn run test.

For any test case with a leak you'll notice a console log (it should be an error after we fix all the leaks on master):

List of sub prs that patches leakages:

• Fixed tests leaking editor instance / DOM element. ckeditor5-build-decoupled-document#31
• Fixed tests leaking editor instance / DOM element. ckeditor5-cloud-services#24
• Fixed tests leaking editor instance / DOM element. ckeditor5-code-block#8
• Fixed tests leaking editor instance / DOM element ckeditor5-core#207 (this PR is a dependency of few others)
• Fixed tests leaking editor instance / DOM element. ckeditor5-editor-balloon#42
• Fixed tests leaking editor instance / DOM element. ckeditor5-editor-classic#102
• Fixed tests leaking editor instance / DOM element. ckeditor5-editor-decoupled#47
• Fixed tests leaking editor instance / DOM element. ckeditor5-editor-inline#64
• Fixed tests leaking editor instance / DOM element. ckeditor5-engine#1812
• Fixed tests leaking editor instance / DOM element. ckeditor5-enter#67
• Fixed tests leaking editor instance / DOM element. ckeditor5-font#58
• Fixed tests leaking editor instance / DOM element. ckeditor5-image#338
• Fixed tests leaking editor instance / DOM element. ckeditor5-table#226
• Fixed tests leaking editor instance / DOM element. ckeditor5-typing#221
• Fixed tests leaking editor instance / DOM element. ckeditor5-utils#318
• Fixed tests leaking editor instance / DOM element. ckeditor5-ui#535
• Fixed tests leaking editor instance / DOM element. ckeditor5-restricted-editing#11
• Fixed tests leaking editor instance / DOM element. ckeditor5-remove-format#15
• Fixed tests leaking editor instance / DOM element. ckeditor5-media-embed#93
• Fixed tests leaking editor instance / DOM element. ckeditor5-list#163
• Fixed tests leaking editor instance / DOM element. ckeditor5-link#253
• Fixed tests leaking editor instance / DOM element. ckeditor5-highlight#47
• Fixed tests leaking editor instance / DOM element ckeditor5-watchdog#32
• Fixed tests leaking editor instance / DOM element ckeditor5-special-characters#10
• Fixed tests leaking editor instance / DOM element ckeditor5-basic-styles#97
• Fixed tests leaking editor instance / DOM element ckeditor5-alignment#45
• Fixed tests leaking editor instance / DOM element ckeditor5-upload#107

[mlewand]

As shown in #6002 (comment) detecting DOM leaks is pretty easy (though I simplified it do check only document.body - since these are the only leaks that happened to me so far). So I'm proposing moving it to our test stack once we get rid of all the leaks (and make it an error instead of a warning).

I do believe that it will also be possible to add an automated check for editor instances leakage (with no need of modifying current tests), although I'll need to verify this idea later on. This would address another part of #409.

[mlewand]

Also personally I wouldn't mind making handling helpers removal easier. E.g. we could add in our tests code that would allow you to call `disposables.add( domRoot )`, `disposables.add( editor )`, `disposables.add( newEditor )` - a stack that would be automatically cleaned up after each test case.
 

That way instead of doing `afterEach` each time we allocate stuff, we only would have to add our editor/DOM element (potentially anything else) `disposables.add( ... )` and not worry about cleaning this up manually. But that's just a nice to have quality of live improvement imo.

[pomek]

All required PRs (#6002 (comment)) have been merged. Closing.