Skip to content

[TRUNK-16209] Limit quarantine disabling to only fetching context #764

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.

Sign up for GitHub

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

Merged
trunk-io merged 3 commits into from
Sep 2, 2025
Merged

[TRUNK-16209] Limit quarantine disabling to only fetching context #764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b387b5e
[TRUNK-16209] Limit quarantine disabling to only fetching context
cmillar-trunk Aug 28, 2025
58dd598
Switch to not affect exit code, add test
cmillar-trunk Aug 29, 2025
96ac30e
Handle randomness in junit mocks
cmillar-trunk Sep 2, 2025
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
22 changes: 22 additions & 0 deletions cli-tests/src/upload.rs
View file
Open in desktop

@gnalh gnalh Aug 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a pretty big change in terms of functionality. Correct me if I'm wrong, but anyone not using quarantining will suddenly see their uploads fail if we have failures in the bundle.

@cmillar-trunk cmillar-trunk Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I figured that fit more into the ethos of having the cli report the same thing your test runner reports. If you think that'd be more confusing for users, I can switch it to ignore the test failures when checking status if quarantining is disabled.

@gnalh gnalh Aug 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah we definitely don't want to break CI for customers. Right now the expectation for users is: "if quarantining is enabled in the repo then we should fail on un-quarantined test failures. If quarantining is disabled in the repo then we should only fail on upload failures"

@cmillar-trunk cmillar-trunk Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'll change to that.

Original file line number Diff line number Diff line change
Expand Up @@ -1752,3 +1752,25 @@ async fn fails_if_sha_is_too_long() {

println!("{assert}");
}

#[tokio::test(flavor = "multi_thread")]
async fn reports_failing_tests_but_succeeds_when_quarantine_disabled() {
let temp_dir = tempdir().unwrap();
generate_mock_git_repo(&temp_dir);
generate_mock_valid_junit_xmls_with_failures(&temp_dir);

let state = MockServerBuilder::new().spawn_mock_server().await;

let mut command = CommandBuilder::upload(temp_dir.path(), state.host.clone())
.disable_quarantining(true)
.command();

// Our junit library generates random amounts of test failures (but at least 1),
// so we check that we don't have 0 failures.
let assert = command
.assert()
.success()
.stderr(predicate::str::contains("Fail: 0").not());

println!("{assert}");
}
4 changes: 2 additions & 2 deletions cli/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,14 +671,14 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
.collect(),
..Default::default()
},
failures: Vec::default(),
failures: failed_tests_extractor.failed_tests().into(),
repo: RepoUrlParts::default(),
org_url_slug: String::default(),
fetch_status: QuarantineFetchStatus::FetchSkipped,
}
} else {
// default to success if no test run result (i.e. `upload`)
QuarantineContext::skip_fetch()
QuarantineContext::skip_fetch(failed_tests_extractor.failed_tests().to_vec())
}
} else {
gather_quarantine_context(
Expand Down
4 changes: 2 additions & 2 deletions cli/src/context_quarantine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ pub struct QuarantineContext {
pub fetch_status: QuarantineFetchStatus,
}
impl QuarantineContext {
pub fn skip_fetch() -> Self {
pub fn skip_fetch(failures: Vec<Test>) -> Self {
Self {
exit_code: i32::default(),
quarantine_status: QuarantineBulkTestStatus::default(),
failures: Vec::default(),
failures,
repo: RepoUrlParts::default(),
org_url_slug: String::default(),
fetch_status: QuarantineFetchStatus::FetchSkipped,
Expand Down
Loading