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

RSpec quarantining support #484

Merged
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
5 changes: 5 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
AllCops:
TargetRubyVersion: 3.1
GlobalVars:
AllowedVariables:
- $test_report
Metrics/PerceivedComplexity:
Max: 15
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 1 addition & 6 deletions api/src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ pub struct TelemetryUploadMetricsRequest {

#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
pub struct PageQuery {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The api updated the names before release.

#[serde(rename = "pageSize")]
pub page_size: i32,
#[serde(rename = "pageToken")]
pub page_token: String,
}

Expand All @@ -85,9 +83,7 @@ pub struct Page {
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
pub struct ListQuarantinedTestsRequest {
pub repo: RepoUrlParts,
#[serde(rename = "orgUrlSlug")]
pub org_url_slug: String,
#[serde(rename = "pageQuery")]
pub page_query: PageQuery,
}

Expand All @@ -104,8 +100,7 @@ pub struct QuarantinedTest {
pub name: String,
pub parent: Option<String>,
pub file: Option<String>,
#[serde(rename = "className")]
pub class_name: Option<String>,
pub classname: Option<String>,
pub status: String,
pub quarantine_setting: QuarantineSetting,
pub test_case_id: String,
Expand Down
1 change: 1 addition & 0 deletions api/src/urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ fn test_url_generated() {
file: None,
id: String::from("c33a7f64-8f3e-5db9-b37b-2ea870d2441b"),
timestamp_millis: None,
is_quarantined: false,
};

let actual = url_for_test_case(
Expand Down
68 changes: 67 additions & 1 deletion bundle/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::hash::{Hash, Hasher};

use context::repo::RepoUrlParts;
#[cfg(feature = "pyo3")]
use pyo3::prelude::*;
Expand All @@ -22,10 +24,18 @@ pub struct Test {
pub id: String,
/// Added in v0.6.9
pub timestamp_millis: Option<i64>,
pub is_quarantined: bool,
}

impl Hash for Test {
fn hash<H: Hasher>(&self, state: &mut H) {
self.id.hash(state);
}
}

impl Test {
pub fn new<T: AsRef<str>>(
id: Option<T>,
name: String,
parent_name: String,
class_name: Option<String>,
Expand All @@ -41,9 +51,14 @@ impl Test {
file,
id: String::with_capacity(0),
timestamp_millis,
is_quarantined: false,
};

test.set_id(org_slug, repo);
if let Some(id) = id {
test.generate_custom_uuid(org_slug.as_ref(), repo, id.as_ref());
} else {
test.set_id(org_slug, repo);
}

test
}
Expand All @@ -62,6 +77,25 @@ impl Test {
self.id =
uuid::Uuid::new_v5(&uuid::Uuid::NAMESPACE_URL, info_id_input.as_bytes()).to_string()
}

pub fn generate_custom_uuid<T: AsRef<str>>(&mut self, org_slug: T, repo: &RepoUrlParts, id: T) {
if id.as_ref().is_empty() {
self.set_id(org_slug.as_ref(), repo);
return;
}
if uuid::Uuid::parse_str(id.as_ref()).is_ok() {
self.id = id.as_ref().to_string();
return;
}
let info_id_input = [
org_slug.as_ref(),
repo.repo_full_name().as_str(),
id.as_ref(),
]
.join("#");
self.id =
uuid::Uuid::new_v5(&uuid::Uuid::NAMESPACE_URL, info_id_input.as_bytes()).to_string();
}
}

#[derive(Debug, Serialize, Clone, Deserialize, Default)]
Expand Down Expand Up @@ -191,6 +225,22 @@ mod tests {
name: "name".to_string(),
};
let result = Test::new(
None,
name.clone(),
parent_name.clone(),
class_name.clone(),
file.clone(),
org_slug,
&repo,
Some(0),
);
assert_eq!(result.name, name);
assert_eq!(result.parent_name, parent_name);
assert_eq!(result.class_name, class_name);
assert_eq!(result.file, file);
assert_eq!(result.id, "aad1f138-09ab-5ea9-9c21-af48a03d6edd");
let result = Test::new(
Some("aad1f138-09ab-5ea9-9c21-af48a03d6edd"),
name.clone(),
parent_name.clone(),
class_name.clone(),
Expand All @@ -204,13 +254,29 @@ mod tests {
assert_eq!(result.class_name, class_name);
assert_eq!(result.file, file);
assert_eq!(result.id, "aad1f138-09ab-5ea9-9c21-af48a03d6edd");
let result = Test::new(
Some("trunk:example-id"),
name.clone(),
parent_name.clone(),
class_name.clone(),
file.clone(),
org_slug,
&repo,
Some(0),
);
assert_eq!(result.name, name);
assert_eq!(result.parent_name, parent_name);
assert_eq!(result.class_name, class_name);
assert_eq!(result.file, file);
assert_eq!(result.id, "208beb01-6179-546e-b0dd-8502e24ae85c");
let result = Test {
name: name.clone(),
parent_name: parent_name.clone(),
class_name: class_name.clone(),
file: file.clone(),
id: String::from("da5b8893-d6ca-5c1c-9a9c-91f40a2a3649"),
timestamp_millis: Some(0),
is_quarantined: false,
};
assert_eq!(result.name, name);
assert_eq!(result.parent_name, parent_name);
Expand Down
14 changes: 14 additions & 0 deletions cli/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,20 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
if let Some(test_run_result) = test_run_result {
QuarantineContext {
exit_code: test_run_result.exit_code,
quarantine_status: QuarantineBulkTestStatus {
quarantine_results: failed_tests_extractor
.failed_tests()
.iter()
.filter_map(|test| {
if test.is_quarantined {
Some(test.clone())
} else {
None
}
})
.collect(),
..Default::default()
},
..Default::default()
}
} else {
Expand Down
3 changes: 2 additions & 1 deletion cli/src/context_quarantine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ fn convert_case_to_test<T: AsRef<str>>(
file,
id: String::with_capacity(0),
timestamp_millis,
is_quarantined: case.is_quarantined(),
};
if let Some(id) = case.extra().get("id") {
if id.is_empty() {
Expand Down Expand Up @@ -279,7 +280,7 @@ pub async fn gather_quarantine_context(
.iter()
.cloned()
.for_each(|failure| {
let quarantine_failure = quarantined.contains(&failure.id);
let quarantine_failure = quarantined.contains(&failure.id) || failure.is_quarantined;
if quarantine_failure {
quarantined_failures.push(failure);
} else {
Expand Down
3 changes: 3 additions & 0 deletions context-ruby/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions context-ruby/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ PATH
remote: .
specs:
rspec_trunk_flaky_tests (0.0.0)
colorize (= 1.1.0)
rb_sys (= 0.9.103)
rspec-core (> 3.3)

GEM
remote: https://rubygems.org/
specs:
colorize (1.1.0)
diff-lcs (1.5.1)
rake (13.2.1)
rake-compiler (1.2.0)
Expand Down
1 change: 1 addition & 0 deletions context-ruby/context_ruby.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Gem::Specification.new do |s|
s.email = '[email protected]'
s.files = Dir['lib/**/*.rb', 'ext/**/*.{rs,rb}', '**/Cargo.*']
s.add_runtime_dependency('rspec-core', '>3.3')
s.add_dependency('colorize', '=1.1.0')
s.add_dependency('rb_sys', '=0.9.103')
s.add_development_dependency('rspec')
s.homepage = 'https://trunk.io'
Expand Down
77 changes: 41 additions & 36 deletions context-ruby/lib/trunk_spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,27 @@
require 'rspec/core'
require 'time'
require 'context_ruby'
require 'colorize'

def escape(str)
str.dump[1..-2]
end

def description_generated?(example)
auto_generated_exp = /^\s?is expected to eq .*$/
full_description = example.full_description
parent_description = example.example_group.description
checked_description = full_description.sub(parent_description, '')
auto_generated_exp.match(checked_description) != nil
end

def generate_id(example)
return "trunk:#{example.id}-#{example.location}" if description_generated?(example)
end

# we want to cache the test report 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(' ')}")

module RSpec
module Core
# Example is the class that represents a test case
Expand All @@ -17,16 +33,23 @@ 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/Naming/AccessorMethodName)
# trunk-ignore(rubocop/Naming/AccessorMethodName,rubocop/Metrics/MethodLength,rubocop/Metrics/AbcSize)
def set_exception(exception)
# TODO: this is where we'll need to override the result once the logic is ready
# trunk-ignore(rubocop/Lint/LiteralAsCondition)
if true
set_exception_core(exception)
else
id = generate_id(self)
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 "Checking if test is quarantined: `#{name}` in `#{parent_name}`".colorize(:yellow)
if $test_report.is_quarantined(id, name, parent_name, classname, file)
# monitor the override in the metadata
metadata[:quarantined_exception] = exception
puts "Test is quarantined, overriding exception: #{exception}".colorize(:green)
nil
else
puts 'Test is not quarantined, continuing'.colorize(:red)
set_exception_core(exception)
end
end

Expand Down Expand Up @@ -70,10 +93,6 @@ def run
else
@example.metadata[:attempt_number] = 0
end

# add the test to the report
# return the report
@testreport
end
end
end
Expand All @@ -82,42 +101,26 @@ def run
# it generates and submits the final test reports
class TrunkAnalyticsListener
def initialize
@testreport = TestReport.new('rspec', "#{$PROGRAM_NAME} #{ARGV.join(' ')}")
@testreport = $test_report
end

def example_finished(notification)
add_test_case(notification.example)
end

def close(_notification)
@testreport.publish
end

def description_generated?(example)
auto_generated_exp = /^\s?is expected to eq .*$/
full_description = example.full_description
parent_description = example.example_group.description
checked_description = full_description.sub(parent_description, '')
auto_generated_exp.match(checked_description) != nil
end

def generate_id(example)
if description_generated?(example)
# trunk-ignore(rubocop/Style/SoleNestedConditional)
return "trunk:#{example.id}-#{example.location}" if description_generated?(example)
res = @testreport.publish
if res
puts 'Flaky tests report upload complete'.colorize(:green)
else
puts 'Failed to publish flaky tests report'.colorize(:red)
end
nil
end

# trunk-ignore(rubocop/Metrics/AbcSize,rubocop/Metrics/MethodLength,rubocop/Metrics/CyclomaticComplexity)
def add_test_case(example)
if example.exception
failure_message = example.exception.message
# failure details is far more robust than the message, but noiser
# if example.exception.backtrace
# failure_details = example.exception.backtrace.join('\n')
# end
end
failure_message = example.exception.to_s if example.exception
failure_message = example.metadata[:quarantined_exception].to_s if example.metadata[:quarantined_exception]
# TODO: should we use concatenated string or alias when auto-generated description?
name = example.full_description
file = escape(example.metadata[:file_path])
Expand All @@ -129,9 +132,11 @@ def add_test_case(example)

attempt_number = example.metadata[:attempt_number] || 0
status = example.execution_result.status.to_s
# set the status to failure, but mark it as quarantined
is_quarantined = example.metadata[:quarantined_exception] ? true : false
case example.execution_result.status
when :passed
status = Status.new('success')
status = is_quarantined ? Status.new('failure') : Status.new('success')
when :failed
status = Status.new('failure')
when :pending
Expand All @@ -140,7 +145,7 @@ def add_test_case(example)
parent_name = example.example_group.metadata[:description]
parent_name = parent_name.empty? ? 'rspec' : parent_name
@testreport.add_test(id, name, classname, file, parent_name, line, status, attempt_number,
started_at, finished_at, failure_message || '')
started_at, finished_at, failure_message || '', is_quarantined)
end
end

Expand Down
Loading