-
Notifications
You must be signed in to change notification settings - Fork 240
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
Get back to Recovering syncing when we haven't sync for a while #3995
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3995 +/- ##
=======================================
Coverage 84.23% 84.24%
=======================================
Files 266 266
Lines 28349 28364 +15
=======================================
+ Hits 23881 23894 +13
- Misses 4468 4470 +2 ☔ View full report in Codecov by Sentry. |
4f08a1f
to
0b79ccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this. I appreciate.
I'm a bit annoyed by the introduction of the new testing
feature: we are trying to remove then, and this PR adds a new one. I'm proposing a solution in my comment. Feel free to question my comment.
Running => Running, | ||
Running { last_time } => { | ||
// We haven't sync for a while so we should go back to recovering | ||
if last_time.elapsed().as_secs() > 1800 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would like to make this (1800
) configurable because it can depend on the homeserver or how the user wants to use this API. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is configurable, then we don't need mock_instant
, nor the testing
feature (we want to remove them, and this patch is adding them). If this is configurable then, we can simply set it to 1 sec, and have a sleep
of 2 secs in the test. It can sound like a bit hacky but it's probably easier to maintain than having a new dependency and a new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it will be used much but it doesn't cost much to make it a parameter, and if it helps removing some maintenance burden in the tests it's probably worth it.
Yeah what you propose for the test is a bit hacky but it works ™️ , and it feels fine at the integration level.
On it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some docs are still missing, I'll clean up later on in the week, probably from Berlin ;)
a1419c3
to
e3d754b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It's indeed much simpler now!
Can you rebase all your patches now? I think we should be good.
06f957a
to
26c8a43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last bit and we are good!
|
||
/// The state machine used to transition between the [`State`]s. | ||
#[derive(Clone, Debug, PartialEq)] | ||
pub struct StateMachine { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move State
inside StateMachine
please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that was my first try, and then I got lost in how to deal with observing the State
and ownership trouble. I can try again but the change will probably be quite more invasive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you allow me to try please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me push my current state. I am not far, but I broke a sync indicator test so I think I messed up something regarding observable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oupsss it seems to break a lot of complement crypto tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was not the case before my last refactor.
Fixes #3935.
Untested in the apps for now, but integration test is included and working.
Signed-off-by: Mathieu Velten mathieu.velten@beta.gouv.fr