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

feat: Mock SystemTime::now() for the tests #5113

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Dec 17, 2023

See commit messages

@iequidoo iequidoo marked this pull request as ready for review December 17, 2023 22:43
src/chat.rs Show resolved Hide resolved
src/tools.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

LGTM - two minor comments

src/tests/verified_chats.rs Show resolved Hide resolved
src/tools.rs Outdated Show resolved Hide resolved
@iequidoo
Copy link
Collaborator Author

iequidoo commented Jan 22, 2024

LGTM - two minor comments

Thanks for reviewing, but finally i think it indeed should be moved to a separate crate, even as is, maybe it will be useful elsewhere. I think that #5108 should be merged first at least to avoid text conflicts.

@iequidoo iequidoo marked this pull request as ready for review January 23, 2024 03:38
@iequidoo
Copy link
Collaborator Author

So, now this fake struct is deltachat_time::SystemTimeTools which is re-exported as tools::SystemTime in the test configuration so that SystemTime::now() is the mocked version. Better ideas are welcome

Add a new crate `deltachat_time` with a fake `struct SystemTimeTools` for mocking
`SystemTime::now()` for test purposes. One still needs to use `std::time::SystemTime` as a struct
representing a system time. I think such a minimalistic approach is ok -- even if somebody uses the
original `SystemTime::now()` instead of the mock by mistake, that could break only tests but not the
program itself. The worst thing that can happen is that tests using `SystemTime::shift()` and
checking messages timestamps f.e. wouldn't catch the corresponding bugs, but now we don't have such
tests at all which is much worse.
Even if `vc-request-with-auth` is received with a delay, the protection message must have the sort
timestamp equal to the Sent timestamp of `vc-request-with-auth`, otherwise subsequent chat messages
would also have greater sort timestamps and while it doesn't affect the chat itself (because Sent
timestamps are shown to a user), it affects the chat position in the chatlist because chats there
are sorted by sort timestamps of the last messages, so the user sees chats sorted out of
order. That's what happened in #5088 where a user restores the backup made before setting up a
verified chat with their contact and fetches new messages, including `vc-request-with-auth` and also
messages from other chats, after that.
@iequidoo iequidoo merged commit 778660a into main Feb 15, 2024
38 checks passed
@iequidoo iequidoo deleted the iequidoo/time-tests branch February 15, 2024 17:24
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.

None yet

3 participants