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

rust: always use persistence in clients #115

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Jul 9, 2024

Previously we would only use persistence when the test strictly needed it e.g to test what happens between restarts. It's preferable to always run with persistence because:

  • it more accurately models real Element X clients e.g performance characteristics, runtime code.
  • any bugs in the DB layer are then also bugs in the client. Using the memory store has proven error prone and fixing these bugs don't improve the stability of EX at all.

ClientCreationOpts will still have the Persistence flag though, as it remains useful to know when a test needs persistence vs when it does not. In the future, we may clean up locally stored files during test runtime, and this flag would allow us to know whether it is safe to delete the files or not.

Previously we would only use persistence when the test strictly needed
it e.g to test what happens between restarts. It's preferable to always
run with persistence because:
 - it more accurately models real Element X clients e.g performance
   characteristics, runtime code.
 - any bugs in the DB layer are then also bugs in the client. Using
   the memory store has proven [error prone](matrix-org/matrix-rust-sdk#3668)
   and fixing these bugs don't improve the stability of EX at all.

`ClientCreationOpts` will still have the `Persistence` flag though,
as it remains useful to know when a test _needs_ persistence vs when
it does not. In the future, we may clean up locally stored files
during test runtime, and this flag would allow us to know whether it
is safe to delete the files or not.
@kegsay kegsay mentioned this pull request Jul 9, 2024
@richvdh richvdh self-requested a review July 9, 2024 10:49
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

shouldn't we remove ClientCreationOpts.PersistentStorage if it no longer does anything?

@kegsay
Copy link
Member Author

kegsay commented Jul 9, 2024

I explain why I've kept it in the PR description.

@kegsay kegsay requested a review from richvdh July 9, 2024 11:49
@richvdh
Copy link
Member

richvdh commented Jul 9, 2024

I see. It might be a good idea to update the doc on PersistentStorage then, to explain that it's an indicator of the test's intent, rather than a setting that will cause any particular defined behaviour?

@kegsay kegsay merged commit d48d6ba into main Jul 9, 2024
4 checks passed
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.

2 participants