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

test: Notifiy more prominently & in more tests about false positives when running cargo test #6308

Merged
merged 2 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4724,7 +4724,7 @@ mod tests {
use crate::headerdef::HeaderDef;
use crate::message::delete_msgs;
use crate::receive_imf::receive_imf;
use crate::test_utils::{sync, TestContext, TestContextManager};
use crate::test_utils::{sync, TestContext, TestContextManager, TimeShiftFalsePositiveNote};
use strum::IntoEnumIterator;
use tokio::fs;

Expand Down Expand Up @@ -5268,6 +5268,8 @@ mod tests {

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_modify_chat_disordered() -> Result<()> {
let _n = TimeShiftFalsePositiveNote;

// Alice creates a group with Bob, Claire and Daisy and then removes Claire and Daisy
// (sleep() is needed as otherwise smeared time from Alice looks to Bob like messages from the future which are all set to "now" then)
let alice = TestContext::new_alice().await;
Expand Down
17 changes: 4 additions & 13 deletions src/contact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1984,7 +1984,7 @@ mod tests {
use crate::chat::{get_chat_contacts, send_text_msg, Chat};
use crate::chatlist::Chatlist;
use crate::receive_imf::receive_imf;
use crate::test_utils::{self, TestContext, TestContextManager};
use crate::test_utils::{self, TestContext, TestContextManager, TimeShiftFalsePositiveNote};

#[test]
fn test_contact_id_values() {
Expand Down Expand Up @@ -2915,6 +2915,8 @@ Hi."#;

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_was_seen_recently() -> Result<()> {
let _n = TimeShiftFalsePositiveNote;

let mut tcm = TestContextManager::new();
let alice = tcm.alice().await;
let bob = tcm.bob().await;
Expand All @@ -2930,18 +2932,7 @@ Hi."#;
bob.recv_msg(&sent_msg).await;
let contact = Contact::get_by_id(&bob, *contacts.first().unwrap()).await?;

let green = nu_ansi_term::Color::Green.normal();
assert!(
contact.was_seen_recently(),
"{}",
green.paint(
"\nNOTE: This test failure is probably a false-positive, caused by tests running in parallel.
The issue is that `SystemTime::shift()` (a utility function for tests) changes the time for all threads doing tests, and not only for the running test.
Until the false-positive is fixed:
- Use `cargo test -- --test-threads 1` instead of `cargo test`
- Or use `cargo nextest run` (install with `cargo install cargo-nextest --locked`)\n"
)
);
assert!(contact.was_seen_recently());

let self_contact = Contact::get_by_id(&bob, ContactId::SELF).await?;
assert!(!self_contact.was_seen_recently());
Expand Down
4 changes: 3 additions & 1 deletion src/securejoin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ mod tests {
use crate::imex::{imex, ImexMode};
use crate::receive_imf::receive_imf;
use crate::stock_str::{self, chat_protection_enabled};
use crate::test_utils::get_chat_msg;
use crate::test_utils::{get_chat_msg, TimeShiftFalsePositiveNote};
use crate::test_utils::{TestContext, TestContextManager};
use crate::tools::SystemTime;
use std::collections::HashSet;
Expand Down Expand Up @@ -798,6 +798,8 @@ mod tests {
}

async fn test_setup_contact_ex(case: SetupContactCase) {
let _n = TimeShiftFalsePositiveNote;

let mut tcm = TestContextManager::new();
let alice = tcm.alice().await;
let alice_addr = &alice.get_config(Config::Addr).await.unwrap().unwrap();
Expand Down
18 changes: 18 additions & 0 deletions src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,24 @@ async fn write_msg(context: &Context, prefix: &str, msg: &Message, buf: &mut Str
.unwrap();
}

/// When dropped after a test failure,
/// prints a note about a possible false-possible caused by SystemTime::shift().
pub(crate) struct TimeShiftFalsePositiveNote;
impl Drop for TimeShiftFalsePositiveNote {
fn drop(&mut self) {
if std::thread::panicking() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may not panic at this point but return Error in some tests also, but idk how to catch that. Anyway, the proposed approach is better than the previous assert! in the only test

let green = nu_ansi_term::Color::Green.normal();
println!("{}", green.paint(
"\nNOTE: This test failure may be a false-positive, caused by tests running in parallel.
The issue is that `SystemTime::shift()` (a utility function for tests) changes the time for all threads doing tests, and not only for the running test.
Until the false-positive is fixed:
- Use `cargo test -- --test-threads 1` instead of `cargo test`
- Or use `cargo nextest run` (install with `cargo install cargo-nextest --locked`)\n")
);
}
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
Loading