Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 14, 2023

  • fix: Emit sync event after successful push to pick up syncing again
  • tests(cypress): Refactor reconnect test to wait more reliably

🚧 TODO

🏁 Checklist

  • 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 (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Jun 14, 2023

2 flaky tests on run #10379 ↗︎

0 147 1 0 Flakiness 2

Details:

Emit sync event after successful push to pick up syncing again
Project: Text Commit: 72dfc69626
Status: Passed Duration: 03:41 💡
Started: Jun 21, 2023 5:32 PM Ended: Jun 21, 2023 5:35 PM
Flakiness  sync.spec.js • 1 flaky test

View Output Video

Test Artifacts
Sync > recovers from a lost connection Output Screenshots
Flakiness  api/UsersApi.spec.js • 1 flaky test

View Output Video

Test Artifacts
The user mention API > fetches users with valid session Output Screenshots

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

@juliusknorr
Copy link
Member Author

Found two other reconnect bug when testing but those would be something for a different round:

  • Open text
  • Set to offline in network tab of browser console
  • Click reconnect
  • 🐛 It seems to show an error but request still happen and there is a continuous loading spinner

  • Open text
  • Delete the oc_text_session entry
  • 🐛 See an empty editor

@juliusknorr
Copy link
Member Author

Still need to figure out how to stabilize the save after reconnect

@max-nextcloud Do you have any suggestion or code pointer how we do that in other cypress tests? Might be worth to look into separating the save request soonish ;)

@max-nextcloud
Copy link
Collaborator

@max-nextcloud Do you have any suggestion or code pointer how we do that in other cypress tests? Might be worth to look into separating the save request soonish ;)

Looks like the save if versions match commit addressed this, right?

We have this in a bunch of places and my usual workaround was to wait for at least two syncs. Used to be so that first sync ensures steps by others are synced and second sync could then make sure we are up to date with our own steps.

I think if we fix the actual issue and maybe separate the save and sync requests we could get rid of a few of those waits.

@max-nextcloud
Copy link
Collaborator

I think if we fix the actual issue and maybe separate the save and sync requests we could get rid of a few of those waits.

The only example of that pattern i could find outside the directediting spec is unrelated:
https://github.com/nextcloud/text/blob/d32b251883057af456679de90322eeb1e716618a/cypress/e2e/nodes/FrontMatter.spec.js#L76C1-L77

So maybe we got rid of them already.

@juliusknorr juliusknorr changed the title bugfix/noid/sync after push reconnect Emit sync event after successful push to pick up syncing again Jun 19, 2023
@juliusknorr juliusknorr added bug Something isn't working 2. developing labels Jun 19, 2023
@juliusknorr juliusknorr added this to the Nextcloud 28 milestone Jun 19, 2023
@juliusknorr juliusknorr force-pushed the bugfix/noid/sync-after-push-reconnect branch from abc342d to 663885b Compare June 19, 2023 19:32
})

it('recovers from a lost connection', () => {
it.only('recovers from a lost connection', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

To remove

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.

Thanks a lot for looking into this @juliushaertl! Code changes look sensible to me (apart from .only in the Cypress test).

@juliusknorr juliusknorr force-pushed the bugfix/noid/sync-after-push-reconnect branch from 663885b to 90d51cc Compare June 20, 2023 10:00
@juliusknorr
Copy link
Member Author

/compile

@juliusknorr juliusknorr force-pushed the bugfix/noid/sync-after-push-reconnect branch from c4c9d50 to 292ee76 Compare June 21, 2023 06:21
@juliusknorr
Copy link
Member Author

/compile

@juliusknorr juliusknorr force-pushed the bugfix/noid/sync-after-push-reconnect branch from 292ee76 to 9225acd Compare June 21, 2023 16:14
@juliusknorr juliusknorr force-pushed the bugfix/noid/sync-after-push-reconnect branch from 9225acd to 25256c6 Compare June 21, 2023 17:11
@juliusknorr
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <[email protected]>
@juliusknorr juliusknorr merged commit a6b5832 into main Jun 21, 2023
@juliusknorr juliusknorr deleted the bugfix/noid/sync-after-push-reconnect branch June 21, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants