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

Restore restarted watchdog editor with current react state #546

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Oct 15, 2024

Suggested merge commit message (convention)

Fix: Prevent potential crashes of useMultiRootEditor during the initialization phase when setting the new state of the multi-root editor with an attached watchdog. Closes #542


Additional information

It appears that some of our tests are causing random restarts of the editor. An example of such a restart might occur when setting some data while the component has not fully rendered the entire CKEditor, and the semaphore begins destroying it while running in the watchdog context.

This PR resets the reinitialized editor data to the one defined in the React state (data is defined in the upper scope). This change seems to make our tests more stable and should be harmless.

@coveralls
Copy link

coveralls commented Oct 15, 2024

Pull Request Test Coverage Report for Build e60bbb33-6ee8-42a4-9a3f-558ebc50367c

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 99f1a8b3-7db2-4b7a-b3cb-6130df28e2ca: 0.0%
Covered Lines: 582
Relevant Lines: 582

💛 - Coveralls

Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a dedicated test for that? Now we have one which sometimes fails due to previous behavior (but it may not), so this specific logic is not really covered 🤔

@Mati365
Copy link
Member Author

Mati365 commented Oct 16, 2024

@f1ames I don't think so, simulating this is too difficult, and it's that kind of behavior that often changes with React implementation changes.

@Mati365 Mati365 requested a review from f1ames October 16, 2024 08:06
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@f1ames I don't think so, simulating this is too difficult, and it's that kind of behavior that often changes with React implementation changes.

Ok, I agree. LGTM then 👍

@Mati365 Mati365 merged commit 4df6509 into master Oct 16, 2024
6 checks passed
@Mati365 Mati365 deleted the ck/542 branch October 16, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky 'useMultiRootEditor' slow editor test
3 participants