ckeditor / ckeditor5-react

Official CKEditor 5 React component.
https://ckeditor.com/ckeditor-5
Other
425 stars 99 forks source link

Flaky 'useMultiRootEditor' slow editor test #542

Closed f1ames closed 1 month ago

f1ames commented 1 month ago

The should assign properly data property to editor even if it is still mounting (see here) test fails randomly with:

- Expected
+ Received

  Object {
    "content": "",
-   "intro": "Hello World!",
+   "intro": "",
  }

 ❯ tests/useMultiRootEditor.test.tsx:951:57
    949|     await waitFor( () => {
    950|      expect( result.current.editor ).to.be.instanceof( SlowEditor );
    951|      expect( result.current.editor!.data.get() ).to.deep.equal( {
       |                                                         ^
    952|       intro: 'Hello World!',
    953|       content: ''

See on CI.

https://github.com/ckeditor/ckeditor5-react/blob/8886ca37769cd5145e6c6249ea5859a60ee9120b/tests/useMultiRootEditor.test.tsx#L903

It might be related to using timeouts:

https://github.com/ckeditor/ckeditor5-react/blob/8886ca37769cd5145e6c6249ea5859a60ee9120b/tests/useMultiRootEditor.test.tsx#L938-L955

What's worth mentioning is that it seems it got more unstable after #534 PR got merged.

f1ames commented 1 month ago

After #543, it seems this is not flaky test in fact, but really a regression from #534. Test is failing randomly still after improvements from #543 so there might be race condition or something similar going on.

f1ames commented 1 month ago

Ok, some initial observations. I'll put my console dump first (you can skip it first and go to conclusions first) which might not be really readable but still (and at least useful for me) and then refer to it.

You can find a diff of the above here - https://www.diffchecker.com/H91iddhW/.

See dumps ### Successful run ``` ----- START ----- useMultiRootEditor:return - editorRefs.instance.current?.id undefined useMultiRootEditor:return - editorRefs.instance.current?.id undefined useLifeCycleSemaphoreSyncRef:runAfterMount - revision 1728908958656 useMultiRootEditor:_initializeEditor - totalRestartsRef { "current": 0, } useMultiRootEditor:_initializeEditor:watchdog.setCreator - totalRestartsRef.current +0 useMultiRootEditor:_externalSetData - newData { "content": "", "intro": "Hello World!", } useLifeCycleSemaphoreSyncRef:runAfterMount - revision 1728908958656 ----- ASSERT ----- useMultiRootEditor:_externalSetData:semaphore.runAfterMount - newData, houldUpdateEditor.current { "content": "", "intro": "Hello World!", } true useMultiRootEditor:useEffect:semaphore.replace:afterMount useMultiRootEditor:useInstantEditorEffect - shouldUpdateEditor.current true useMultiRootEditor:useInstantEditorEffect - data { "content": "", "intro": "Hello World!", } useMultiRootEditor:useInstantEditorEffect - instance.data.get(), instance.id {} eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:useInstantEditorEffect - editorRefs.instance.current?.data.get(), editorRefs.instance.current?.id {} eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:useInstantEditorEffect - roots, instance.model.document.getRoots() [ "intro", "content", "footer", ] [ "intro", "content", "footer", ] useMultiRootEditor:useInstantEditorEffect - newRoots [] useMultiRootEditor:useInstantEditorEffect - removedRoots [ "footer", ] useMultiRootEditor:return - editorRefs.instance.current?.id eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:return - editorRefs.instance.current?.id eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:useInstantEditorEffect:_updateEditorData - dataToUpdate { "content": "", "intro": "Hello World!", } useMultiRootEditor:onDetachRoot - rootName footer useMultiRootEditor:onChangeData - newData {} useMultiRootEditor:useInstantEditorEffect - shouldUpdateEditor.current false useMultiRootEditor:useInstantEditorEffect - data { "content": "", "intro": "Hello World!", } useMultiRootEditor:useInstantEditorEffect - instance.data.get(), instance.id { "content": "", "intro": "Hello World!", } eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:useInstantEditorEffect - editorRefs.instance.current?.data.get(), editorRefs.instance.current?.id { "content": "", "intro": "Hello World!", } eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:useInstantEditorEffect - roots, instance.model.document.getRoots() [ "intro", "content", ] [ "intro", "content", ] useMultiRootEditor:return - editorRefs.instance.current?.id eab447c86dc59195b19d8fefbfd95f8e7 useMultiRootEditor:return - editorRefs.instance.current?.id eab447c86dc59195b19d8fefbfd95f8e7 ----- ASSERT ----- { "content": "", "intro": "Hello World!", } eab447c86dc59195b19d8fefbfd95f8e7 ``` ### Failing run ``` ----- START ----- useMultiRootEditor:return - editorRefs.instance.current?.id undefined useMultiRootEditor:return - editorRefs.instance.current?.id undefined useLifeCycleSemaphoreSyncRef:runAfterMount - revision 1728909075935 useMultiRootEditor:return - editorRefs.instance.current?.id undefined useMultiRootEditor:return - editorRefs.instance.current?.id undefined useMultiRootEditor:_initializeEditor - totalRestartsRef { "current": 0, } useMultiRootEditor:_initializeEditor:watchdog.setCreator - totalRestartsRef.current +0 useMultiRootEditor:_externalSetData - newData { "content": "", "intro": "Hello World!", } useLifeCycleSemaphoreSyncRef:runAfterMount - revision 1728909075935 ----- ASSERT ----- useMultiRootEditor:useInstantEditorEffect - shouldUpdateEditor.current true useMultiRootEditor:useInstantEditorEffect - data {} useMultiRootEditor:useInstantEditorEffect - instance.data.get(), instance.id {} ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - editorRefs.instance.current?.data.get(), editorRefs.instance.current?.id {} ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - roots, instance.model.document.getRoots() [ "intro", "content", "footer", ] [ "intro", "content", "footer", ] useMultiRootEditor:useInstantEditorEffect - newRoots [] useMultiRootEditor:useInstantEditorEffect - removedRoots [ "intro", "content", "footer", ] useMultiRootEditor:_externalSetData:semaphore.runAfterMount - newData, houldUpdateEditor.current { "content": "", "intro": "Hello World!", } false useMultiRootEditor:useEffect:semaphore.replace:afterMount useMultiRootEditor:useInstantEditorEffect - shouldUpdateEditor.current true useMultiRootEditor:useInstantEditorEffect - data { "content": "", "intro": "Hello World!", } useMultiRootEditor:useInstantEditorEffect - instance.data.get(), instance.id {} ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - editorRefs.instance.current?.data.get(), editorRefs.instance.current?.id {} ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - roots, instance.model.document.getRoots() [ "intro", "content", "footer", ] [ "intro", "content", "footer", ] useMultiRootEditor:useInstantEditorEffect - newRoots [] useMultiRootEditor:useInstantEditorEffect - removedRoots [ "footer", ] useMultiRootEditor:return - editorRefs.instance.current?.id ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:return - editorRefs.instance.current?.id ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect:_updateEditorData - dataToUpdate { "content": "", "intro": "Hello World!", } useMultiRootEditor:onDetachRoot - rootName footer useMultiRootEditor:onChangeData - newData {} useMultiRootEditor:useInstantEditorEffect - shouldUpdateEditor.current false useMultiRootEditor:useInstantEditorEffect - data { "content": "", "intro": "Hello World!", } useMultiRootEditor:useInstantEditorEffect - instance.data.get(), instance.id { "content": "", "intro": "Hello World!", } ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - editorRefs.instance.current?.data.get(), editorRefs.instance.current?.id { "content": "", "intro": "Hello World!", } ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:useInstantEditorEffect - roots, instance.model.document.getRoots() [ "intro", "content", ] [ "intro", "content", ] useMultiRootEditor:return - editorRefs.instance.current?.id ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:return - editorRefs.instance.current?.id ee9e8e2bde0ca16fea929a15e86555d27 useMultiRootEditor:_initializeEditor:watchdog.on_error [CKEditorError: writer-detachroot-no-root Read more: https://ckeditor.com/docs/ckeditor5/latest/support/error-codes.html#error-writer-detachroot-no-root] useMultiRootEditor:_initializeEditor:watchdog.setCreator - totalRestartsRef.current 1 useMultiRootEditor:return - editorRefs.instance.current?.id ead7e49e7b97f24d64f968654f434db89 ----- ASSERT ----- { "content": "", "intro": "", } ead7e49e7b97f24d64f968654f434db89 ----- ASSERT ----- { "content": "", "intro": "", } ead7e49e7b97f24d64f968654f434db89 ```

Some conclusions

  1. 535 PR changed the content of those tests slightly by adding additional footer root (see https://github.com/ckeditor/ckeditor5-react/pull/535/files#diff-5b81880d0441ee0751e0a44635f33052c1176a93a78f4ef4914b85e42adc265fR23).

  2. The test fails because of watchdog restarting the editor after setting data. Watchdog restart is caused by the editor crash because of #error-writer-detachroot-no-root when footer root is about to be detached. And so assert gets data from new editor instance - see diff screenshot below (important - I would assume after editor is restarted by watchdog it should have the same contents than previous instance, either it's broken too or race condition here so data is not yet set on new instance).

Image

  1. The failing run starts by some empty set data (see diff screenshot below) which detaches all the roots, and so next footer detach fails (here I'm still looking into why it happens, looks like trying to detach the same root multiple times is also some outdated state issue).

Image

  1. When I run this test in separation (both watchdog [true, false]) it fails like 30%. When I run the test only with watchdog (by modifying this for) it seems to fail always. Running it with "npx vitest run -t 'should assign properly data property to editor even if it is still mounting'".
  2. It is not a regression from #535. This issue can be observed before those changes by adding additional root to these tests. I created a branch from v9.3.0 and modified the test and it fails the same way. See the branch - https://github.com/ckeditor/ckeditor5-react/tree/t/542-test (you should run only this test, others will fail since I did not adjusted assertions there - "npx vitest run -t 'should assign properly data property to editor even if it is still mounting'").
  3. This tests mocks console.warn and console.error to empty functions, which makes noticing errors (like the one from this issue) really time consuming.
f1ames commented 1 month ago

@Mati365 I guess it was closed incidentally, right? We concluded that #543 does not fix it.

Mati365 commented 1 month ago

Yep, sorry.