From 858ed3d16974c86b213e7c871c360dc7a160a399 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 15 Dec 2023 23:19:37 -0300 Subject: [PATCH 1/2] feat: Mock SystemTime::now() for the tests 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. --- Cargo.lock | 5 +++++ Cargo.toml | 2 ++ deltachat-time/Cargo.toml | 8 ++++++++ deltachat-time/src/lib.rs | 35 +++++++++++++++++++++++++++++++++++ src/chat.rs | 6 +++--- src/contact.rs | 4 ++-- src/context.rs | 4 ++-- src/ephemeral.rs | 4 ++-- src/sql.rs | 6 +++--- src/sync.rs | 3 ++- src/tests/verified_chats.rs | 8 ++++---- src/timesmearing.rs | 6 +++--- src/tools.rs | 6 +++++- 13 files changed, 76 insertions(+), 21 deletions(-) create mode 100644 deltachat-time/Cargo.toml create mode 100644 deltachat-time/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 9fa5797662..fdd15ac635 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1103,6 +1103,7 @@ dependencies = [ "brotli", "chrono", "criterion", + "deltachat-time", "deltachat_derive", "email", "encoded-words", @@ -1223,6 +1224,10 @@ dependencies = [ "yerpc", ] +[[package]] +name = "deltachat-time" +version = "1.0.0" + [[package]] name = "deltachat_derive" version = "2.0.0" diff --git a/Cargo.toml b/Cargo.toml index 7c54a48ed0..461b76dfd0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ strip = true [dependencies] deltachat_derive = { path = "./deltachat_derive" } +deltachat-time = { path = "./deltachat-time" } format-flowed = { path = "./format-flowed" } ratelimit = { path = "./deltachat-ratelimit" } @@ -128,6 +129,7 @@ members = [ "deltachat-rpc-server", "deltachat-ratelimit", "deltachat-repl", + "deltachat-time", "format-flowed", ] diff --git a/deltachat-time/Cargo.toml b/deltachat-time/Cargo.toml new file mode 100644 index 0000000000..a2931d7263 --- /dev/null +++ b/deltachat-time/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "deltachat-time" +version = "1.0.0" +description = "Time-related tools" +edition = "2021" +license = "MPL-2.0" + +[dependencies] diff --git a/deltachat-time/src/lib.rs b/deltachat-time/src/lib.rs new file mode 100644 index 0000000000..c8d7d6f6c2 --- /dev/null +++ b/deltachat-time/src/lib.rs @@ -0,0 +1,35 @@ +#![allow(missing_docs)] + +use std::sync::RwLock; +use std::time::{Duration, SystemTime}; + +static SYSTEM_TIME_SHIFT: RwLock = RwLock::new(Duration::new(0, 0)); + +/// Fake struct for mocking `SystemTime::now()` for test purposes. You still need to use +/// `SystemTime` as a struct representing a system time. +pub struct SystemTimeTools(); + +impl SystemTimeTools { + pub const UNIX_EPOCH: SystemTime = SystemTime::UNIX_EPOCH; + + pub fn now() -> SystemTime { + return SystemTime::now() + *SYSTEM_TIME_SHIFT.read().unwrap(); + } + + /// Simulates a system clock forward adjustment by `duration`. + pub fn shift(duration: Duration) { + *SYSTEM_TIME_SHIFT.write().unwrap() += duration; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() { + SystemTimeTools::shift(Duration::from_secs(60)); + let t = SystemTimeTools::now(); + assert!(t > SystemTime::now()); + } +} diff --git a/src/chat.rs b/src/chat.rs index 87b8163c3f..a084add082 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -6,7 +6,7 @@ use std::convert::{TryFrom, TryInto}; use std::fmt; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::time::{Duration, SystemTime}; +use std::time::Duration; use anyhow::{anyhow, bail, ensure, Context as _, Result}; use deltachat_derive::{FromSql, ToSql}; @@ -44,7 +44,7 @@ use crate::sync::{self, Sync::*, SyncData}; use crate::tools::{ buf_compress, create_id, create_outgoing_rfc724_mid, create_smeared_timestamp, create_smeared_timestamps, get_abs_path, gm2local_offset, improve_single_line_input, - smeared_time, strip_rtlo_characters, time, IsNoneOrEmpty, + smeared_time, strip_rtlo_characters, time, IsNoneOrEmpty, SystemTime, }; use crate::webxdc::WEBXDC_SUFFIX; @@ -3656,7 +3656,7 @@ pub enum MuteDuration { Forever, /// Chat is muted for a limited period of time. - Until(SystemTime), + Until(std::time::SystemTime), } impl rusqlite::types::ToSql for MuteDuration { diff --git a/src/contact.rs b/src/contact.rs index 49f85435cd..70db10b15f 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -6,7 +6,7 @@ use std::convert::{TryFrom, TryInto}; use std::fmt; use std::ops::Deref; use std::path::{Path, PathBuf}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::UNIX_EPOCH; use anyhow::{bail, ensure, Context as _, Result}; use async_channel::{self as channel, Receiver, Sender}; @@ -36,7 +36,7 @@ use crate::sql::{self, params_iter}; use crate::sync::{self, Sync::*}; use crate::tools::{ duration_to_str, get_abs_path, improve_single_line_input, strip_rtlo_characters, time, - EmailAddress, + EmailAddress, SystemTime, }; use crate::{chat, stock_str}; diff --git a/src/context.rs b/src/context.rs index 3f3365f516..a297146d64 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1217,7 +1217,7 @@ pub fn get_version_str() -> &'static str { #[cfg(test)] mod tests { - use std::time::{Duration, SystemTime}; + use std::time::Duration; use anyhow::Context as _; use strum::IntoEnumIterator; @@ -1234,7 +1234,7 @@ mod tests { use crate::mimeparser::SystemMessage; use crate::receive_imf::receive_imf; use crate::test_utils::{get_chat_msg, TestContext}; - use crate::tools::create_outgoing_rfc724_mid; + use crate::tools::{create_outgoing_rfc724_mid, SystemTime}; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_wrong_db() -> Result<()> { diff --git a/src/ephemeral.rs b/src/ephemeral.rs index 48e0a1ab16..719243f3e1 100644 --- a/src/ephemeral.rs +++ b/src/ephemeral.rs @@ -67,7 +67,7 @@ use std::collections::BTreeSet; use std::convert::{TryFrom, TryInto}; use std::num::ParseIntError; use std::str::FromStr; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::{Duration, UNIX_EPOCH}; use anyhow::{ensure, Result}; use async_channel::Receiver; @@ -85,7 +85,7 @@ use crate::message::{Message, MessageState, MsgId, Viewtype}; use crate::mimeparser::SystemMessage; use crate::sql::{self, params_iter}; use crate::stock_str; -use crate::tools::{duration_to_str, time}; +use crate::tools::{duration_to_str, time, SystemTime}; /// Ephemeral timer value. #[derive(Debug, PartialEq, Eq, Copy, Clone, Serialize, Deserialize)] diff --git a/src/sql.rs b/src/sql.rs index 9333ba1614..2909eab2d5 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -21,7 +21,7 @@ use crate::message::{Message, MsgId, Viewtype}; use crate::param::{Param, Params}; use crate::peerstate::{deduplicate_peerstates, Peerstate}; use crate::stock_str; -use crate::tools::{delete_file, time}; +use crate::tools::{delete_file, time, SystemTime}; /// Extension to [`rusqlite::ToSql`] trait /// which also includes [`Send`] and [`Sync`]. @@ -850,9 +850,9 @@ pub async fn remove_unused_files(context: &Context) -> Result<()> { Ok(mut dir_handle) => { /* avoid deletion of files that are just created to build a message object */ let diff = std::time::Duration::from_secs(60 * 60); - let keep_files_newer_than = std::time::SystemTime::now() + let keep_files_newer_than = SystemTime::now() .checked_sub(diff) - .unwrap_or(std::time::SystemTime::UNIX_EPOCH); + .unwrap_or(SystemTime::UNIX_EPOCH); while let Ok(Some(entry)) = dir_handle.next_entry().await { let name_f = entry.file_name(); diff --git a/src/sync.rs b/src/sync.rs index e719562a52..b63f919f9d 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -319,7 +319,7 @@ impl Context { #[cfg(test)] mod tests { - use std::time::{Duration, SystemTime}; + use std::time::Duration; use anyhow::bail; @@ -329,6 +329,7 @@ mod tests { use crate::contact::{Contact, Origin}; use crate::test_utils::TestContext; use crate::token::Namespace; + use crate::tools::SystemTime; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_config_sync_msgs() -> Result<()> { diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index 06adeba0ad..a07e093ebc 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -12,6 +12,7 @@ use crate::mimeparser::SystemMessage; use crate::receive_imf::receive_imf; use crate::stock_str; use crate::test_utils::{get_chat_msg, mark_as_verified, TestContext, TestContextManager}; +use crate::tools::SystemTime; use crate::{e2ee, message}; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -792,10 +793,9 @@ async fn test_create_protected_grp_multidev() -> Result<()> { ); let sent = alice.send_text(group_id, "Hey").await; - // This sleep is necessary to reproduce the bug when the original message is sorted over the - // "protection enabled" message so that these messages have different timestamps. The better way - // would be to adjust the system time here if we could mock the system clock for the tests. - tokio::time::sleep(std::time::Duration::from_millis(2000)).await; + // This time shift is necessary to reproduce the bug when the original message is sorted over + // the "protection enabled" message so that these messages have different timestamps. + SystemTime::shift(std::time::Duration::from_secs(3600)); let msg = alice1.recv_msg(&sent).await; let group1 = Chat::load_from_db(alice1, msg.chat_id).await?; assert_eq!(group1.get_type(), Chattype::Group); diff --git a/src/timesmearing.rs b/src/timesmearing.rs index 21c3c57f8a..6bd4dbbba0 100644 --- a/src/timesmearing.rs +++ b/src/timesmearing.rs @@ -79,11 +79,11 @@ impl SmearedTimestamp { #[cfg(test)] mod tests { - use std::time::SystemTime; - use super::*; use crate::test_utils::TestContext; - use crate::tools::{create_smeared_timestamp, create_smeared_timestamps, smeared_time, time}; + use crate::tools::{ + create_smeared_timestamp, create_smeared_timestamps, smeared_time, time, SystemTime, + }; #[test] fn test_smeared_timestamp() { diff --git a/src/tools.rs b/src/tools.rs index 7563ab1396..8652995224 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -15,12 +15,16 @@ use std::str::from_utf8; // while being in deep sleep mode, we use `SystemTime` instead, but add an alias for it to document // why `Instant` isn't used in those places. Also this can help to switch to another clock impl if // we find any. +use std::time::Duration; pub use std::time::SystemTime as Time; -use std::time::{Duration, SystemTime}; +#[cfg(not(test))] +pub use std::time::SystemTime; use anyhow::{bail, Context as _, Result}; use base64::Engine as _; use chrono::{Local, NaiveDateTime, NaiveTime, TimeZone}; +#[cfg(test)] +pub use deltachat_time::SystemTimeTools as SystemTime; use futures::{StreamExt, TryStreamExt}; use mailparse::dateparse; use mailparse::headers::Headers; From 38a6c8ce5a31dd76e5f92a677efc28e288b9c1eb Mon Sep 17 00:00:00 2001 From: iequidoo Date: Sun, 17 Dec 2023 17:05:08 -0300 Subject: [PATCH 2/2] test: Add a test on protection message sort timestamp (#5088) 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. --- src/securejoin.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/securejoin.rs b/src/securejoin.rs index dc2321ee98..8758460a06 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -770,10 +770,20 @@ mod tests { use crate::stock_str::chat_protection_enabled; use crate::test_utils::get_chat_msg; use crate::test_utils::{TestContext, TestContextManager}; - use crate::tools::EmailAddress; + use crate::tools::{EmailAddress, SystemTime}; + use std::time::Duration; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_setup_contact() { + test_setup_contact_ex(false).await + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_setup_contact_protection_timestamp() { + test_setup_contact_ex(true).await + } + + async fn test_setup_contact_ex(check_protection_timestamp: bool) { let mut tcm = TestContextManager::new(); let alice = tcm.alice().await; let bob = tcm.bob().await; @@ -862,6 +872,10 @@ mod tests { // Check Bob sent the right message. let sent = bob.pop_sent_msg().await; let msg = alice.parse_msg(&sent).await; + let vc_request_with_auth_ts_sent = msg + .get_header(HeaderDef::Date) + .and_then(|value| mailparse::dateparse(value).ok()) + .unwrap(); assert!(msg.was_encrypted()); assert_eq!( msg.get_header(HeaderDef::SecureJoin).unwrap(), @@ -885,6 +899,10 @@ mod tests { .unwrap(); assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), false); + if check_protection_timestamp { + SystemTime::shift(Duration::from_secs(3600)); + } + // Step 5+6: Alice receives vc-request-with-auth, sends vc-contact-confirm alice.recv_msg(&sent).await; assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), true); @@ -910,6 +928,9 @@ mod tests { assert!(msg.is_info()); let expected_text = chat_protection_enabled(&alice).await; assert_eq!(msg.get_text(), expected_text); + if check_protection_timestamp { + assert_eq!(msg.timestamp_sort, vc_request_with_auth_ts_sent); + } } // Check Alice sent the right message to Bob.