Skip to content

Conversation

@max-nextcloud
Copy link
Collaborator

📝 Summary

When we recover a session the editor already exists.
Make sure editing can be resumed:

  • Start the syncing again as the polling backend is created from scratch.

  • Do not attach further events to the editor

  • Do not reset the content.

  • Resolves: Test failures in sync spec

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation is not required

@max-nextcloud
Copy link
Collaborator Author

/backport 5dbd7bd to stable26

@cypress
Copy link

cypress bot commented Mar 5, 2023

2 flaky tests on run #8903 ↗︎

0 140 0 0 Flakiness 2

Details:

fix: only initialize editor once
Project: Text Commit: 2d2c300f7f
Status: Passed Duration: 03:17 💡
Started: Mar 6, 2023 7:33 AM Ended: Mar 6, 2023 7:37 AM
Flakiness  cypress/e2e/sync.spec.js • 2 flaky tests

View Output Video

Test Artifacts
Sync > saves the actual file and document state Output Screenshots
Sync > recovers from a lost connection Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Code looks sensible to me.

@max-nextcloud max-nextcloud force-pushed the fix/recovery-sync-test branch from 5dbd7bd to eeee735 Compare March 5, 2023 19:10
@max-nextcloud
Copy link
Collaborator Author

/compile

@mejo-
Copy link
Member

mejo- commented Mar 5, 2023

Is the failing Cypress test the one you would expect to have fixed with this PR?

  1. Sync
    recovers from a lost connection:
    AssertionError: Timed out retrying after 10000ms: expected '## Hello world\n\n- Saving the doc saves the doc state\n- ' to include 'after the lost connection'
    at Context.eval (webpack://@nextcloud/text/./cypress/e2e/sync.spec.js:92:4)

@max-nextcloud
Copy link
Collaborator Author

Is the failing Cypress test the one you would expect to have fixed with this PR?

Yes. I saw it fail occasionally locally - but three failures in a row is way too much. Will need to investigate some more.

max-nextcloud and others added 2 commits March 6, 2023 08:30
When we recover a session the editor already exists.
Start the syncing again as the polling backend is created from scratch.
Do not attach further events to the editor and do not reset the content.

Signed-off-by: Max <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@max-nextcloud max-nextcloud force-pushed the fix/recovery-sync-test branch from 7cda8e1 to 2d2c300 Compare March 6, 2023 07:30
@max-nextcloud
Copy link
Collaborator Author

I made the tests more robust i think. Let's see if they turn flaky eventually. The actual fix to the underlying problem would be #3899 .

@max-nextcloud max-nextcloud merged commit fbacdf7 into main Mar 6, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/recovery-sync-test branch March 6, 2023 07:42
@backportbot-nextcloud
Copy link

The backport to stable26 failed. Please do this backport manually.

@juliusknorr
Copy link
Member

back ported manually with #3900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review 26 feedback bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants