diff --git a/.rubocop.yml b/.rubocop.yml index a5675b37..6931e1a8 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -3,5 +3,6 @@ AllCops: GlobalVars: AllowedVariables: - $test_report + - $failure_encountered_and_quarantining_disabled Metrics/PerceivedComplexity: Max: 15 diff --git a/rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb b/rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb index aed6d02f..5e12c49a 100644 --- a/rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb +++ b/rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb @@ -75,6 +75,7 @@ def trunk_disabled # 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 $test_report = TestReport.new('rspec', "#{$PROGRAM_NAME} #{ARGV.join(' ')}", nil) +$failure_encountered_and_quarantining_disabled = false module RSpec module Core @@ -86,20 +87,34 @@ class Example # RSpec uses the existance of an exception to determine if the test failed # We need to override this to allow us to capture the exception and then # decide if we want to fail the test or not - # trunk-ignore(rubocop/Metrics/AbcSize,rubocop/Metrics/MethodLength,rubocop/Naming/AccessorMethodName) + # trunk-ignore(rubocop/Naming/AccessorMethodName) def set_exception(exception) return set_exception_core(exception) if metadata[:pending] return set_exception_core(exception) if trunk_disabled return set_exception_core(exception) if metadata[:retry_attempts]&.positive? + handle_quarantine_check(exception) + end + + # trunk-ignore(rubocop/Metrics/AbcSize,rubocop/Metrics/MethodLength) + def handle_quarantine_check(exception) id = generate_trunk_id name = full_description parent_name = example_group.metadata[:description] parent_name = parent_name.empty? ? 'rspec' : parent_name file = escape(metadata[:file_path]) classname = file.sub(%r{\.[^/.]+\Z}, '').gsub('/', '.').gsub(/\A\.+|\.+\Z/, '') - puts "Test failed, checking if it can be quarantined: `#{location}`".yellow - if $test_report.is_quarantined(id, name, parent_name, classname, file) + unless $failure_encountered_and_quarantining_disabled + puts "Test failed, checking if it can be quarantined: `#{location}`".yellow + end + is_quarantined_result = $test_report.is_quarantined(id, name, parent_name, classname, file) + if is_quarantined_result.quarantining_disabled_for_repo + unless $failure_encountered_and_quarantining_disabled + puts 'Quarantining is disabled for this repo, no failures will be quarantined'.yellow + $failure_encountered_and_quarantining_disabled = true + end + set_exception_core(exception) + elsif is_quarantined_result.test_is_quarantined # monitor the override in the metadata metadata[:quarantined_exception] = exception puts "Test is quarantined, overriding exception: #{exception}".green @@ -182,6 +197,9 @@ def example_finished(notification) # trunk-ignore(rubocop/Metrics/MethodLength) def close(_notification) + if $failure_encountered_and_quarantining_disabled + puts 'Note: Quarantining is disabled for this repo. Test failures were not quarantined.'.yellow + end if ENV['TRUNK_LOCAL_UPLOAD_DIR'] saved = @testreport.try_save(ENV['TRUNK_LOCAL_UPLOAD_DIR']) if saved diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 512e4011..89cb4b30 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -27,9 +27,15 @@ use uuid::Uuid; #[cfg(feature = "wasm")] use wasm_bindgen::prelude::wasm_bindgen; -#[derive(Debug, Clone, Serialize, Deserialize)] -struct QuarantinedTestsDiskCacheEntry { +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct QuarantineConfig { + quarantining_disabled_for_repo: bool, quarantined_tests: HashMap, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct QuarantineConfigDiskCacheEntry { + quarantine_config: QuarantineConfig, cached_at_secs: u64, } @@ -38,13 +44,46 @@ pub struct TestReport { test_report: TestReportProto, command: String, started_at: SystemTime, - quarantined_tests: Option>, + quarantine_config: Option, quarantined_tests_disk_cache_ttl: Duration, codeowners: Option, variant: Option, repo: Option, } +#[cfg_attr(feature = "wasm", wasm_bindgen(getter_with_clone))] +#[cfg_attr(feature = "ruby", magnus::wrap(class = "IsQuarantinedResult"))] +#[derive(Debug, Clone, Copy, PartialEq, Default)] +pub struct IsQuarantinedResult { + pub test_is_quarantined: bool, + pub quarantining_disabled_for_repo: bool, +} + +#[cfg(feature = "ruby")] +impl IsQuarantinedResult { + pub fn test_is_quarantined(&self) -> bool { + self.test_is_quarantined + } + + pub fn quarantining_disabled_for_repo(&self) -> bool { + self.quarantining_disabled_for_repo + } +} + +impl From for bool { + fn from(val: IsQuarantinedResult) -> Self { + val.test_is_quarantined && !val.quarantining_disabled_for_repo + } +} + +impl std::ops::Not for IsQuarantinedResult { + type Output = bool; + + fn not(self) -> Self::Output { + !self.test_is_quarantined || self.quarantining_disabled_for_repo + } +} + #[cfg_attr(feature = "wasm", wasm_bindgen)] #[cfg_attr(feature = "ruby", magnus::wrap(class = "Status"))] #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -162,7 +201,7 @@ impl MutTestReport { test_report, command, started_at, - quarantined_tests: None, + quarantine_config: None, quarantined_tests_disk_cache_ttl, codeowners, repo, @@ -256,16 +295,16 @@ impl MutTestReport { parent_name: Option, classname: Option, file: Option, - ) -> bool { + ) -> IsQuarantinedResult { let token = self.get_token(); let org_url_slug = self.get_org_url_slug(); if token.is_empty() { tracing::warn!("Not checking quarantine status because TRUNK_API_TOKEN is empty"); - return false; + return IsQuarantinedResult::default(); } if org_url_slug.is_empty() { tracing::warn!("Not checking quarantine status because TRUNK_ORG_URL_SLUG is empty"); - return false; + return IsQuarantinedResult::default(); } let api_client = ApiClient::new(token, org_url_slug.clone(), None); let use_uncloned_repo = env::var(constants::TRUNK_USE_UNCLONED_REPO_ENV) @@ -302,51 +341,57 @@ impl MutTestReport { bundle_repo.repo_url, org_url_slug, ); - if let Some(quarantined_tests) = self.0.borrow().quarantined_tests.as_ref() { - return quarantined_tests.get(&test_identifier.id).is_some(); + if let Some(quarantine_config) = self.0.borrow().quarantine_config.as_ref() { + return IsQuarantinedResult { + test_is_quarantined: quarantine_config + .quarantined_tests + .contains_key(&test_identifier.id), + quarantining_disabled_for_repo: quarantine_config + .quarantining_disabled_for_repo, + }; } - false + IsQuarantinedResult::default() } _ => { tracing::warn!("Unable to fetch quarantined tests"); - false + IsQuarantinedResult::default() } } } - fn get_quarantined_tests_cache_file_path(&self, org_url_slug: &str, repo_url: &str) -> PathBuf { + fn get_quarantine_config_cache_file_path(&self, org_url_slug: &str, repo_url: &str) -> PathBuf { let cache_key = Uuid::new_v5( &Uuid::NAMESPACE_URL, format!("{org_url_slug}#{repo_url}").as_bytes(), ) .to_string(); - let quarantined_tests_cache_file_name = format!("quarantined_tests_{cache_key}.json"); + let quarantine_config_cache_file_name = format!("quarantine_config_{cache_key}.json"); env::temp_dir() .join(constants::CACHE_DIR) - .join(quarantined_tests_cache_file_name) + .join(quarantine_config_cache_file_name) } - fn load_quarantined_tests_from_disk_cache( + fn load_quarantine_config_from_disk_cache( &self, org_url_slug: &str, repo_url: &str, - ) -> Option> { - let cache_path = self.get_quarantined_tests_cache_file_path(org_url_slug, repo_url); + ) -> Option { + let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url); let cache_file = match fs::File::open(&cache_path) { Ok(file) => file, Err(err) => { - tracing::warn!("Failed to open quarantined tests cache file: {:?}", err); + tracing::warn!("Failed to open quarantine config cache file: {:?}", err); return None; } }; - let cache_entry: QuarantinedTestsDiskCacheEntry = match serde_json::from_reader(cache_file) + let cache_entry: QuarantineConfigDiskCacheEntry = match serde_json::from_reader(cache_file) { Ok(entry) => entry, Err(err) => { - tracing::warn!("Failed to parse quarantined tests cache file: {:?}", err); + tracing::warn!("Failed to parse quarantine config cache file: {:?}", err); return None; } }; @@ -361,20 +406,20 @@ impl MutTestReport { let cache_age = now.saturating_sub(cache_entry.cached_at_secs); if cache_age < self.0.borrow().quarantined_tests_disk_cache_ttl.as_secs() { - Some(cache_entry.quarantined_tests) + Some(cache_entry) } else { let _ = fs::remove_file(&cache_path); None } } - fn save_quarantined_tests_to_disk_cache( + fn save_quarantine_config_to_disk_cache( &self, org_url_slug: &str, repo_url: &str, - quarantined_tests: &HashMap, + quarantine_config: &QuarantineConfig, ) { - let cache_path = self.get_quarantined_tests_cache_file_path(org_url_slug, repo_url); + let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url); let now = match SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) { Ok(duration) => duration.as_secs(), @@ -384,8 +429,8 @@ impl MutTestReport { } }; - let cache_entry = QuarantinedTestsDiskCacheEntry { - quarantined_tests: quarantined_tests.clone(), + let cache_entry = QuarantineConfigDiskCacheEntry { + quarantine_config: quarantine_config.clone(), cached_at_secs: now, }; @@ -404,7 +449,7 @@ impl MutTestReport { if let Ok(json) = serde_json::to_string(&cache_entry) { if let Err(err) = fs::write(&cache_path, json) { - tracing::warn!("Failed to write quarantined tests cache file: {:?}", err); + tracing::warn!("Failed to write quarantine config cache file: {:?}", err); } } } @@ -416,14 +461,14 @@ impl MutTestReport { repo_url: String, org_url_slug: String, ) { - if self.0.borrow().quarantined_tests.as_ref().is_some() { + if self.0.borrow().quarantine_config.is_some() { return; } - if let Some(quarantined_tests) = - self.load_quarantined_tests_from_disk_cache(&org_url_slug, &repo_url) + if let Some(cache_entry) = + self.load_quarantine_config_from_disk_cache(&org_url_slug, &repo_url) { - self.0.borrow_mut().quarantined_tests = Some(quarantined_tests); + self.0.borrow_mut().quarantine_config = Some(cache_entry.quarantine_config); return; } @@ -439,11 +484,13 @@ impl MutTestReport { .build() .unwrap() .block_on(api_client.get_quarantining_config(&request)); - match response { + let quarantining_disabled = match response { Ok(response) => { + let is_disabled = response.is_disabled; for quarantined_test_id in response.quarantined_tests.iter() { quarantined_tests.insert(quarantined_test_id.clone(), true); } + is_disabled } Err(err) => { tracing::warn!("Unable to fetch quarantined tests"); @@ -452,11 +499,16 @@ impl MutTestReport { "Error fetching quarantined tests: {:?}", err ); + false } - } + }; - self.0.borrow_mut().quarantined_tests = Some(quarantined_tests.clone()); - self.save_quarantined_tests_to_disk_cache(&org_url_slug, &repo_url, &quarantined_tests); + let quarantine_config = QuarantineConfig { + quarantining_disabled_for_repo: quarantining_disabled, + quarantined_tests: quarantined_tests.clone(), + }; + self.0.borrow_mut().quarantine_config = Some(quarantine_config.clone()); + self.save_quarantine_config_to_disk_cache(&org_url_slug, &repo_url, &quarantine_config); } fn get_org_url_slug(&self) -> String { @@ -740,6 +792,15 @@ pub fn ruby_init(ruby: &magnus::Ruby) -> Result<(), magnus::Error> { let status = ruby.define_class("Status", ruby.class_object())?; status.define_singleton_method("new", magnus::function!(Status::new, 1))?; status.define_method("to_s", magnus::method!(Status::to_string, 0))?; + let is_quarantined_result = ruby.define_class("IsQuarantinedResult", ruby.class_object())?; + is_quarantined_result.define_method( + "test_is_quarantined", + magnus::method!(IsQuarantinedResult::test_is_quarantined, 0), + )?; + is_quarantined_result.define_method( + "quarantining_disabled_for_repo", + magnus::method!(IsQuarantinedResult::quarantining_disabled_for_repo, 0), + )?; let test_report = ruby.define_class("TestReport", ruby.class_object())?; test_report.define_singleton_method("new", magnus::function!(MutTestReport::new, 3))?; test_report.define_method("to_s", magnus::method!(MutTestReport::to_string, 0))?; diff --git a/test_report/tests/report.rs b/test_report/tests/report.rs index 1c991e0e..ce677d35 100644 --- a/test_report/tests/report.rs +++ b/test_report/tests/report.rs @@ -625,6 +625,10 @@ async fn test_variant_impacts_quarantining() { is_quarantined_v1, "Test should be quarantined when variant matches (without ID)" ); + assert!( + !is_quarantined_v1.quarantining_disabled_for_repo, + "Quarantining should not be disabled for the repo" + ); // Test with variant1 - should find the quarantined test (with ID) env::set_var(TRUNK_VARIANT_ENV, "variant1");