Skip to content

Commit aefab7b

Browse files
jrrayjrray
committed
Another attempt to fix spurious test failures
Following up on #1052 there are still spurious test failures in other tests, e.g., sync::sync_test::test_sync_ref_including_annotation_blob, that fail because the spfs config is set to legacy encoding format. These tests that change the config were not putting the config back to defaults, so that could explain the spurious failures depending on the order that tests run. Although, it is suspicious that the tests weren't failing more often if this is the cause. Signed-off-by: J Robert Ray <[email protected]>
1 parent 8b75563 commit aefab7b

File tree

4 files changed

+516
-430
lines changed

4 files changed

+516
-430
lines changed

‎crates/spfs/src/config_test.rs‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use rstest::rstest;
77
use super::{Config, Remote, RemoteConfig, RepositoryConfig};
88
use crate::storage::RepositoryHandle;
99
use crate::storage::prelude::*;
10-
use crate::{get_config, load_config};
10+
use crate::{get_config, load_config, reset_config};
1111

1212
#[rstest]
1313
fn test_config_list_remote_names_empty() {
@@ -121,17 +121,19 @@ fn test_config_expands_tilde_in_paths() {
121121
#[rstest]
122122
#[serial_test::serial(config)]
123123
fn test_make_current_updates_config() {
124-
let config1 = Config::default();
125-
config1.make_current().unwrap();
124+
reset_config! {
125+
let config1 = Config::default();
126+
config1.make_current().unwrap();
126127

127-
let changed_name = "changed";
128+
let changed_name = "changed";
128129

129-
let mut config2 = Config::default();
130-
changed_name.clone_into(&mut config2.user.name);
131-
config2.make_current().unwrap();
130+
let mut config2 = Config::default();
131+
changed_name.clone_into(&mut config2.user.name);
132+
config2.make_current().unwrap();
132133

133-
let current_config = get_config().unwrap();
134-
assert_eq!(current_config.user.name, changed_name);
134+
let current_config = get_config().unwrap();
135+
assert_eq!(current_config.user.name, changed_name);
136+
}
135137
}
136138

137139
#[rstest]

‎crates/spfs/src/graph/layer_test.rs‎

Lines changed: 129 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,72 @@ fn test_layer_encoding_manifest_only() {
2222
assert_eq!(actual.digest().unwrap(), expected.digest().unwrap())
2323
}
2424

25+
/// A macro to reset the global config after running a block of code,
26+
/// even if that block of code panics.
27+
#[macro_export]
28+
macro_rules! reset_config {
29+
($($body:tt)*) => {{
30+
let reset_config_original = Config::current().unwrap();
31+
let _ = std::panic::catch_unwind(|| {
32+
$($body)*
33+
}).unwrap_or_else(|e| {
34+
(*reset_config_original).clone().make_current().unwrap();
35+
36+
std::panic::resume_unwind(e);
37+
});
38+
(*reset_config_original).clone().make_current().unwrap();
39+
}};
40+
}
41+
42+
/// Sanity test to ensure that the reset_config macro catches panics
43+
#[rstest]
44+
#[should_panic]
45+
fn reset_config_catches_panics() {
46+
reset_config! {
47+
panic!("This is a test panic");
48+
49+
#[allow(unreachable_code)]
50+
Ok::<(), ()>(())
51+
};
52+
}
53+
54+
/// A macro to reset the global config after running a block of code,
55+
/// even if that block of code panics.
56+
#[macro_export]
57+
macro_rules! reset_config_async {
58+
($($body:tt)*) => {{
59+
let reset_config_original = Config::current().unwrap();
60+
let reset_config_handle = tokio::task::spawn(async move {
61+
$($body)*
62+
});
63+
let result = reset_config_handle.await;
64+
(*reset_config_original).clone().make_current().unwrap();
65+
match result {
66+
Ok(_) => {}
67+
Err(err) if err.is_panic() => {
68+
let err = err.into_panic();
69+
std::panic::resume_unwind(err);
70+
}
71+
Err(err) => {
72+
panic!("Task failed to complete: {err}");
73+
}
74+
}
75+
}};
76+
}
77+
78+
/// Sanity test to ensure that the reset_config_async macro catches panics
79+
#[rstest]
80+
#[should_panic]
81+
#[tokio::test]
82+
async fn reset_config_async_catches_panics() {
83+
reset_config_async! {
84+
panic!("This is a test panic");
85+
86+
#[allow(unreachable_code)]
87+
Ok::<(), ()>(())
88+
};
89+
}
90+
2591
#[rstest(
2692
write_encoding_format => [EncodingFormat::Legacy, EncodingFormat::FlatBuffers],
2793
write_digest_strategy => [DigestStrategy::Legacy, DigestStrategy::WithKindAndSalt],
@@ -31,34 +97,36 @@ fn test_layer_encoding_annotation_only(
3197
write_encoding_format: EncodingFormat,
3298
write_digest_strategy: DigestStrategy,
3399
) {
34-
let mut config = Config::default();
35-
config.storage.encoding_format = write_encoding_format;
36-
config.storage.digest_strategy = write_digest_strategy;
37-
config.make_current().unwrap();
100+
reset_config! {
101+
let mut config = Config::default();
102+
config.storage.encoding_format = write_encoding_format;
103+
config.storage.digest_strategy = write_digest_strategy;
104+
config.make_current().unwrap();
38105

39-
let expected = Layer::new_with_annotation("key", AnnotationValue::string("value"));
106+
let expected = Layer::new_with_annotation("key", AnnotationValue::string("value"));
40107

41-
let mut stream = Vec::new();
42-
match expected.encode(&mut stream) {
43-
Ok(_) if write_encoding_format == EncodingFormat::Legacy => {
44-
panic!("Encode should fail if encoding format is legacy")
45-
}
46-
Ok(_) => {}
47-
Err(_) if write_encoding_format == EncodingFormat::Legacy => {
48-
// This error is expected
49-
return;
50-
}
51-
Err(e) => {
52-
panic!("Error encoding layer: {e}")
53-
}
54-
};
108+
let mut stream = Vec::new();
109+
match expected.encode(&mut stream) {
110+
Ok(_) if write_encoding_format == EncodingFormat::Legacy => {
111+
panic!("Encode should fail if encoding format is legacy")
112+
}
113+
Ok(_) => {}
114+
Err(_) if write_encoding_format == EncodingFormat::Legacy => {
115+
// This error is expected
116+
return;
117+
}
118+
Err(e) => {
119+
panic!("Error encoding layer: {e}")
120+
}
121+
};
55122

56-
let decoded = Object::decode(&mut stream.as_slice());
123+
let decoded = Object::decode(&mut stream.as_slice());
57124

58-
let actual = decoded.unwrap().into_layer().unwrap();
59-
println!(" Actual: {:?}", actual);
125+
let actual = decoded.unwrap().into_layer().unwrap();
126+
println!(" Actual: {:?}", actual);
60127

61-
assert_eq!(actual.digest().unwrap(), expected.digest().unwrap())
128+
assert_eq!(actual.digest().unwrap(), expected.digest().unwrap())
129+
};
62130
}
63131

64132
#[rstest(
@@ -70,46 +138,48 @@ fn test_layer_encoding_manifest_and_annotations(
70138
write_encoding_format: EncodingFormat,
71139
write_digest_strategy: DigestStrategy,
72140
) {
73-
let mut config = Config::default();
74-
config.storage.encoding_format = write_encoding_format;
75-
config.storage.digest_strategy = write_digest_strategy;
76-
config.make_current().unwrap();
141+
reset_config! {
142+
let mut config = Config::default();
143+
config.storage.encoding_format = write_encoding_format;
144+
config.storage.digest_strategy = write_digest_strategy;
145+
config.make_current().unwrap();
77146

78-
let expected = Layer::new_with_manifest_and_annotations(
79-
encoding::EMPTY_DIGEST.into(),
80-
vec![("key", AnnotationValue::string("value"))],
81-
);
82-
println!("Expected: {:?}", expected);
147+
let expected = Layer::new_with_manifest_and_annotations(
148+
encoding::EMPTY_DIGEST.into(),
149+
vec![("key", AnnotationValue::string("value"))],
150+
);
151+
println!("Expected: {:?}", expected);
83152

84-
let mut stream = Vec::new();
85-
match expected.encode(&mut stream) {
86-
Ok(_) if write_encoding_format == EncodingFormat::Legacy => {
87-
panic!("Encode should fail if encoding format is legacy")
88-
}
89-
Ok(_) => {}
90-
Err(_) if write_encoding_format == EncodingFormat::Legacy => {
91-
// This error is expected
92-
return;
93-
}
94-
Err(e) => {
95-
panic!("Error encoding layer: {e}")
96-
}
97-
};
153+
let mut stream = Vec::new();
154+
match expected.encode(&mut stream) {
155+
Ok(_) if write_encoding_format == EncodingFormat::Legacy => {
156+
panic!("Encode should fail if encoding format is legacy")
157+
}
158+
Ok(_) => {}
159+
Err(_) if write_encoding_format == EncodingFormat::Legacy => {
160+
// This error is expected
161+
return;
162+
}
163+
Err(e) => {
164+
panic!("Error encoding layer: {e}")
165+
}
166+
};
98167

99-
let actual = Object::decode(&mut stream.as_slice())
100-
.unwrap()
101-
.into_layer()
102-
.unwrap();
103-
println!(" Actual: {:?}", actual);
168+
let actual = Object::decode(&mut stream.as_slice())
169+
.unwrap()
170+
.into_layer()
171+
.unwrap();
172+
println!(" Actual: {:?}", actual);
104173

105-
match write_encoding_format {
106-
EncodingFormat::Legacy => {
107-
unreachable!();
108-
}
109-
EncodingFormat::FlatBuffers => {
110-
// Under flatbuffers encoding both will contain the
111-
// annotation data and will match
112-
assert_eq!(actual.digest().unwrap(), expected.digest().unwrap())
174+
match write_encoding_format {
175+
EncodingFormat::Legacy => {
176+
unreachable!();
177+
}
178+
EncodingFormat::FlatBuffers => {
179+
// Under flatbuffers encoding both will contain the
180+
// annotation data and will match
181+
assert_eq!(actual.digest().unwrap(), expected.digest().unwrap())
182+
}
113183
}
114184
}
115185
}

0 commit comments

Comments
 (0)