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

Allow configurable sync timeout #7198

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Sep 21, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Allows the sync timeout to be configured via the MatrixConfiguration

Motivation and context

Our instrumentation tests can use a shorter timeout (5 seconds instead of 30 seconds)

Before: 23m 43s
After: 16m 39s

30% reduction in runtime, saving around 7 minutes~

Screenshots / GIFs

Before After
2022-09-21T15:28:49,352822323+01:00 2022-09-21T15:03:35,811981115+01:00

Tests

Run the ./gradlew matrix-sdk-android:connectedAndroidTest, or all the matrix-sdk-android androidTest tests from within Android Studio

Tested devices

  • Physical
  • Emulator
  • OS version(s): 31

@ouchadam ouchadam added matrix-sdk PR-Small PR with less than 20 updated lines labels Sep 21, 2022
@ouchadam ouchadam force-pushed the feature/adm/configurable-sync-timeout branch 2 times, most recently from ec7bbe0 to 6f7ad01 Compare September 21, 2022 14:36
@ouchadam ouchadam requested review from a team and ericdecanini and removed request for a team September 21, 2022 14:38
@ouchadam ouchadam force-pushed the feature/adm/configurable-sync-timeout branch from 5835649 to d75e379 Compare September 21, 2022 14:39
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks. Faster tests!
OOI do you know why the tests are faster? Do we have to wait for a sync request to terminate to finish each test doing sync request? Because if there thing on the server for the client, the client should have a fast answer.

Small remark: you could use more trailing comma (not blocking though).

@ouchadam
Copy link
Contributor Author

I'm not entirely sure to be honest... I suspect you're right and there's some test cases waiting for a sync to finish without there being a change by the server or possibly test cross over state, but I haven't been able to pin point it

here's 3 runs of E2eeSanityTests and they're quite consistent

30s 5s
2022-09-21T16:15:03,469320679+01:00 2022-09-21T16:17:14,344465031+01:00
30s 5s
2022-09-21T16:30:11,678439968+01:00 2022-09-21T16:20:47,963298924+01:00
30s 5s
2022-09-21T16:33:44,771794128+01:00 2022-09-21T16:26:24,981986641+01:00

Small remark: you could use more trailing comma (not blocking though).

will add 👍


the next PR will make them even faster! the switch to coroutines has them around 8mins total~ (still haven't updated all of them yet)

2022-09-21T15:57:33,224261492+01:00

@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ouchadam ouchadam merged commit 65156a8 into develop Sep 22, 2022
@ouchadam ouchadam deleted the feature/adm/configurable-sync-timeout branch September 22, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matrix-sdk PR-Small PR with less than 20 updated lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants