Skip to content

Commit 5fbfd20

Browse files
authored
Cleanup rspec plugin output for when quarantining is disabled for the repo (#966)
[TRUNK-17241](https://linear.app/trunk/issue/TRUNK-17241/improve-rspec-plugin-output-when-quarantining-is-disabled-via-repo) Cleans up the rspec plugin output for when quarantining is disabled for the repo. Re-iterating that this doesn't change any functionality, only messaging / output Previously, when quarantining was disabled via repo settings, we'd still print `Test failed, checking if it can be quarantined: ...` before **every** test failure even though we never quarantine these failures. Now, when quarantining is disabled, we only print this **once** when we encounter the first failure (when we make the network call to `/getQuarantineConfig`) before printing that `Quarantining is disabled for this repo, no failures will be quarantined`. A note re-emphasizing this is also printed at the end of the invocation for clarity. Tested with a locally-built gem: <details><summary>Example run with quarantining disabled & 2 test failures</summary> <img width="714" height="753" alt="Screenshot 2026-01-02 at 2 18 40 PM" src="https://github.com/user-attachments/assets/fd5c91ea-14f6-441a-b32d-e945d5a83c7e" /> </details> <details><summary>Example run with quarantining enabled & 2 test failures</summary> <img width="715" height="658" alt="Screenshot 2026-01-02 at 3 18 34 PM" src="https://github.com/user-attachments/assets/68b197a4-ce01-421b-9438-1a8b4bb41a0d" /> </details> Also updates the caching logic to include the `quarantining_disabled_for_repo` signal returned from `/getQuarantineConfig`
1 parent d87ead8 commit 5fbfd20

File tree

4 files changed

+122
-38
lines changed

4 files changed

+122
-38
lines changed

.rubocop.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,6 @@ AllCops:
33
GlobalVars:
44
AllowedVariables:
55
- $test_report
6+
- $failure_encountered_and_quarantining_disabled
67
Metrics/PerceivedComplexity:
78
Max: 15

rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ def trunk_disabled
7575

7676
# we want to cache the test report in memory so we can add to it as we go and reduce the number of API calls
7777
$test_report = TestReport.new('rspec', "#{$PROGRAM_NAME} #{ARGV.join(' ')}", nil)
78+
$failure_encountered_and_quarantining_disabled = false
7879

7980
module RSpec
8081
module Core
@@ -86,20 +87,34 @@ class Example
8687
# RSpec uses the existance of an exception to determine if the test failed
8788
# We need to override this to allow us to capture the exception and then
8889
# decide if we want to fail the test or not
89-
# trunk-ignore(rubocop/Metrics/AbcSize,rubocop/Metrics/MethodLength,rubocop/Naming/AccessorMethodName)
90+
# trunk-ignore(rubocop/Naming/AccessorMethodName)
9091
def set_exception(exception)
9192
return set_exception_core(exception) if metadata[:pending]
9293
return set_exception_core(exception) if trunk_disabled
9394
return set_exception_core(exception) if metadata[:retry_attempts]&.positive?
9495

96+
handle_quarantine_check(exception)
97+
end
98+
99+
# trunk-ignore(rubocop/Metrics/AbcSize,rubocop/Metrics/MethodLength)
100+
def handle_quarantine_check(exception)
95101
id = generate_trunk_id
96102
name = full_description
97103
parent_name = example_group.metadata[:description]
98104
parent_name = parent_name.empty? ? 'rspec' : parent_name
99105
file = escape(metadata[:file_path])
100106
classname = file.sub(%r{\.[^/.]+\Z}, '').gsub('/', '.').gsub(/\A\.+|\.+\Z/, '')
101-
puts "Test failed, checking if it can be quarantined: `#{location}`".yellow
102-
if $test_report.is_quarantined(id, name, parent_name, classname, file)
107+
unless $failure_encountered_and_quarantining_disabled
108+
puts "Test failed, checking if it can be quarantined: `#{location}`".yellow
109+
end
110+
is_quarantined_result = $test_report.is_quarantined(id, name, parent_name, classname, file)
111+
if is_quarantined_result.quarantining_disabled_for_repo
112+
unless $failure_encountered_and_quarantining_disabled
113+
puts 'Quarantining is disabled for this repo, no failures will be quarantined'.yellow
114+
$failure_encountered_and_quarantining_disabled = true
115+
end
116+
set_exception_core(exception)
117+
elsif is_quarantined_result.test_is_quarantined
103118
# monitor the override in the metadata
104119
metadata[:quarantined_exception] = exception
105120
puts "Test is quarantined, overriding exception: #{exception}".green
@@ -182,6 +197,9 @@ def example_finished(notification)
182197

183198
# trunk-ignore(rubocop/Metrics/MethodLength)
184199
def close(_notification)
200+
if $failure_encountered_and_quarantining_disabled
201+
puts 'Note: Quarantining is disabled for this repo. Test failures were not quarantined.'.yellow
202+
end
185203
if ENV['TRUNK_LOCAL_UPLOAD_DIR']
186204
saved = @testreport.try_save(ENV['TRUNK_LOCAL_UPLOAD_DIR'])
187205
if saved

test_report/src/report.rs

Lines changed: 96 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,15 @@ use uuid::Uuid;
2727
#[cfg(feature = "wasm")]
2828
use wasm_bindgen::prelude::wasm_bindgen;
2929

30-
#[derive(Debug, Clone, Serialize, Deserialize)]
31-
struct QuarantinedTestsDiskCacheEntry {
30+
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
31+
struct QuarantineConfig {
32+
quarantining_disabled_for_repo: bool,
3233
quarantined_tests: HashMap<String, bool>,
34+
}
35+
36+
#[derive(Debug, Clone, Serialize, Deserialize)]
37+
struct QuarantineConfigDiskCacheEntry {
38+
quarantine_config: QuarantineConfig,
3339
cached_at_secs: u64,
3440
}
3541

@@ -38,13 +44,46 @@ pub struct TestReport {
3844
test_report: TestReportProto,
3945
command: String,
4046
started_at: SystemTime,
41-
quarantined_tests: Option<HashMap<String, bool>>,
47+
quarantine_config: Option<QuarantineConfig>,
4248
quarantined_tests_disk_cache_ttl: Duration,
4349
codeowners: Option<CodeOwners>,
4450
variant: Option<String>,
4551
repo: Option<BundleRepo>,
4652
}
4753

54+
#[cfg_attr(feature = "wasm", wasm_bindgen(getter_with_clone))]
55+
#[cfg_attr(feature = "ruby", magnus::wrap(class = "IsQuarantinedResult"))]
56+
#[derive(Debug, Clone, Copy, PartialEq, Default)]
57+
pub struct IsQuarantinedResult {
58+
pub test_is_quarantined: bool,
59+
pub quarantining_disabled_for_repo: bool,
60+
}
61+
62+
#[cfg(feature = "ruby")]
63+
impl IsQuarantinedResult {
64+
pub fn test_is_quarantined(&self) -> bool {
65+
self.test_is_quarantined
66+
}
67+
68+
pub fn quarantining_disabled_for_repo(&self) -> bool {
69+
self.quarantining_disabled_for_repo
70+
}
71+
}
72+
73+
impl From<IsQuarantinedResult> for bool {
74+
fn from(val: IsQuarantinedResult) -> Self {
75+
val.test_is_quarantined && !val.quarantining_disabled_for_repo
76+
}
77+
}
78+
79+
impl std::ops::Not for IsQuarantinedResult {
80+
type Output = bool;
81+
82+
fn not(self) -> Self::Output {
83+
!self.test_is_quarantined || self.quarantining_disabled_for_repo
84+
}
85+
}
86+
4887
#[cfg_attr(feature = "wasm", wasm_bindgen)]
4988
#[cfg_attr(feature = "ruby", magnus::wrap(class = "Status"))]
5089
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -162,7 +201,7 @@ impl MutTestReport {
162201
test_report,
163202
command,
164203
started_at,
165-
quarantined_tests: None,
204+
quarantine_config: None,
166205
quarantined_tests_disk_cache_ttl,
167206
codeowners,
168207
repo,
@@ -256,16 +295,16 @@ impl MutTestReport {
256295
parent_name: Option<String>,
257296
classname: Option<String>,
258297
file: Option<String>,
259-
) -> bool {
298+
) -> IsQuarantinedResult {
260299
let token = self.get_token();
261300
let org_url_slug = self.get_org_url_slug();
262301
if token.is_empty() {
263302
tracing::warn!("Not checking quarantine status because TRUNK_API_TOKEN is empty");
264-
return false;
303+
return IsQuarantinedResult::default();
265304
}
266305
if org_url_slug.is_empty() {
267306
tracing::warn!("Not checking quarantine status because TRUNK_ORG_URL_SLUG is empty");
268-
return false;
307+
return IsQuarantinedResult::default();
269308
}
270309
let api_client = ApiClient::new(token, org_url_slug.clone(), None);
271310
let use_uncloned_repo = env::var(constants::TRUNK_USE_UNCLONED_REPO_ENV)
@@ -302,51 +341,57 @@ impl MutTestReport {
302341
bundle_repo.repo_url,
303342
org_url_slug,
304343
);
305-
if let Some(quarantined_tests) = self.0.borrow().quarantined_tests.as_ref() {
306-
return quarantined_tests.get(&test_identifier.id).is_some();
344+
if let Some(quarantine_config) = self.0.borrow().quarantine_config.as_ref() {
345+
return IsQuarantinedResult {
346+
test_is_quarantined: quarantine_config
347+
.quarantined_tests
348+
.contains_key(&test_identifier.id),
349+
quarantining_disabled_for_repo: quarantine_config
350+
.quarantining_disabled_for_repo,
351+
};
307352
}
308-
false
353+
IsQuarantinedResult::default()
309354
}
310355
_ => {
311356
tracing::warn!("Unable to fetch quarantined tests");
312-
false
357+
IsQuarantinedResult::default()
313358
}
314359
}
315360
}
316361

317-
fn get_quarantined_tests_cache_file_path(&self, org_url_slug: &str, repo_url: &str) -> PathBuf {
362+
fn get_quarantine_config_cache_file_path(&self, org_url_slug: &str, repo_url: &str) -> PathBuf {
318363
let cache_key = Uuid::new_v5(
319364
&Uuid::NAMESPACE_URL,
320365
format!("{org_url_slug}#{repo_url}").as_bytes(),
321366
)
322367
.to_string();
323-
let quarantined_tests_cache_file_name = format!("quarantined_tests_{cache_key}.json");
368+
let quarantine_config_cache_file_name = format!("quarantine_config_{cache_key}.json");
324369

325370
env::temp_dir()
326371
.join(constants::CACHE_DIR)
327-
.join(quarantined_tests_cache_file_name)
372+
.join(quarantine_config_cache_file_name)
328373
}
329374

330-
fn load_quarantined_tests_from_disk_cache(
375+
fn load_quarantine_config_from_disk_cache(
331376
&self,
332377
org_url_slug: &str,
333378
repo_url: &str,
334-
) -> Option<HashMap<String, bool>> {
335-
let cache_path = self.get_quarantined_tests_cache_file_path(org_url_slug, repo_url);
379+
) -> Option<QuarantineConfigDiskCacheEntry> {
380+
let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url);
336381

337382
let cache_file = match fs::File::open(&cache_path) {
338383
Ok(file) => file,
339384
Err(err) => {
340-
tracing::warn!("Failed to open quarantined tests cache file: {:?}", err);
385+
tracing::warn!("Failed to open quarantine config cache file: {:?}", err);
341386
return None;
342387
}
343388
};
344389

345-
let cache_entry: QuarantinedTestsDiskCacheEntry = match serde_json::from_reader(cache_file)
390+
let cache_entry: QuarantineConfigDiskCacheEntry = match serde_json::from_reader(cache_file)
346391
{
347392
Ok(entry) => entry,
348393
Err(err) => {
349-
tracing::warn!("Failed to parse quarantined tests cache file: {:?}", err);
394+
tracing::warn!("Failed to parse quarantine config cache file: {:?}", err);
350395
return None;
351396
}
352397
};
@@ -361,20 +406,20 @@ impl MutTestReport {
361406
let cache_age = now.saturating_sub(cache_entry.cached_at_secs);
362407

363408
if cache_age < self.0.borrow().quarantined_tests_disk_cache_ttl.as_secs() {
364-
Some(cache_entry.quarantined_tests)
409+
Some(cache_entry)
365410
} else {
366411
let _ = fs::remove_file(&cache_path);
367412
None
368413
}
369414
}
370415

371-
fn save_quarantined_tests_to_disk_cache(
416+
fn save_quarantine_config_to_disk_cache(
372417
&self,
373418
org_url_slug: &str,
374419
repo_url: &str,
375-
quarantined_tests: &HashMap<String, bool>,
420+
quarantine_config: &QuarantineConfig,
376421
) {
377-
let cache_path = self.get_quarantined_tests_cache_file_path(org_url_slug, repo_url);
422+
let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url);
378423

379424
let now = match SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) {
380425
Ok(duration) => duration.as_secs(),
@@ -384,8 +429,8 @@ impl MutTestReport {
384429
}
385430
};
386431

387-
let cache_entry = QuarantinedTestsDiskCacheEntry {
388-
quarantined_tests: quarantined_tests.clone(),
432+
let cache_entry = QuarantineConfigDiskCacheEntry {
433+
quarantine_config: quarantine_config.clone(),
389434
cached_at_secs: now,
390435
};
391436

@@ -404,7 +449,7 @@ impl MutTestReport {
404449

405450
if let Ok(json) = serde_json::to_string(&cache_entry) {
406451
if let Err(err) = fs::write(&cache_path, json) {
407-
tracing::warn!("Failed to write quarantined tests cache file: {:?}", err);
452+
tracing::warn!("Failed to write quarantine config cache file: {:?}", err);
408453
}
409454
}
410455
}
@@ -416,14 +461,14 @@ impl MutTestReport {
416461
repo_url: String,
417462
org_url_slug: String,
418463
) {
419-
if self.0.borrow().quarantined_tests.as_ref().is_some() {
464+
if self.0.borrow().quarantine_config.is_some() {
420465
return;
421466
}
422467

423-
if let Some(quarantined_tests) =
424-
self.load_quarantined_tests_from_disk_cache(&org_url_slug, &repo_url)
468+
if let Some(cache_entry) =
469+
self.load_quarantine_config_from_disk_cache(&org_url_slug, &repo_url)
425470
{
426-
self.0.borrow_mut().quarantined_tests = Some(quarantined_tests);
471+
self.0.borrow_mut().quarantine_config = Some(cache_entry.quarantine_config);
427472
return;
428473
}
429474

@@ -439,11 +484,13 @@ impl MutTestReport {
439484
.build()
440485
.unwrap()
441486
.block_on(api_client.get_quarantining_config(&request));
442-
match response {
487+
let quarantining_disabled = match response {
443488
Ok(response) => {
489+
let is_disabled = response.is_disabled;
444490
for quarantined_test_id in response.quarantined_tests.iter() {
445491
quarantined_tests.insert(quarantined_test_id.clone(), true);
446492
}
493+
is_disabled
447494
}
448495
Err(err) => {
449496
tracing::warn!("Unable to fetch quarantined tests");
@@ -452,11 +499,16 @@ impl MutTestReport {
452499
"Error fetching quarantined tests: {:?}",
453500
err
454501
);
502+
false
455503
}
456-
}
504+
};
457505

458-
self.0.borrow_mut().quarantined_tests = Some(quarantined_tests.clone());
459-
self.save_quarantined_tests_to_disk_cache(&org_url_slug, &repo_url, &quarantined_tests);
506+
let quarantine_config = QuarantineConfig {
507+
quarantining_disabled_for_repo: quarantining_disabled,
508+
quarantined_tests: quarantined_tests.clone(),
509+
};
510+
self.0.borrow_mut().quarantine_config = Some(quarantine_config.clone());
511+
self.save_quarantine_config_to_disk_cache(&org_url_slug, &repo_url, &quarantine_config);
460512
}
461513

462514
fn get_org_url_slug(&self) -> String {
@@ -740,6 +792,15 @@ pub fn ruby_init(ruby: &magnus::Ruby) -> Result<(), magnus::Error> {
740792
let status = ruby.define_class("Status", ruby.class_object())?;
741793
status.define_singleton_method("new", magnus::function!(Status::new, 1))?;
742794
status.define_method("to_s", magnus::method!(Status::to_string, 0))?;
795+
let is_quarantined_result = ruby.define_class("IsQuarantinedResult", ruby.class_object())?;
796+
is_quarantined_result.define_method(
797+
"test_is_quarantined",
798+
magnus::method!(IsQuarantinedResult::test_is_quarantined, 0),
799+
)?;
800+
is_quarantined_result.define_method(
801+
"quarantining_disabled_for_repo",
802+
magnus::method!(IsQuarantinedResult::quarantining_disabled_for_repo, 0),
803+
)?;
743804
let test_report = ruby.define_class("TestReport", ruby.class_object())?;
744805
test_report.define_singleton_method("new", magnus::function!(MutTestReport::new, 3))?;
745806
test_report.define_method("to_s", magnus::method!(MutTestReport::to_string, 0))?;

test_report/tests/report.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,10 @@ async fn test_variant_impacts_quarantining() {
625625
is_quarantined_v1,
626626
"Test should be quarantined when variant matches (without ID)"
627627
);
628+
assert!(
629+
!is_quarantined_v1.quarantining_disabled_for_repo,
630+
"Quarantining should not be disabled for the repo"
631+
);
628632

629633
// Test with variant1 - should find the quarantined test (with ID)
630634
env::set_var(TRUNK_VARIANT_ENV, "variant1");

0 commit comments

Comments
 (0)