Skip to content

Commit 1cbbb03

Browse files
authored
Use /getQuarantineConfig for rspec (#932)
Part of [TRUNK-17215](https://linear.app/trunk/issue/TRUNK-17215/cli-validate-rspec-api-lifetime-cache-calls) Swaps usage of `/list-quarantined-tests` with `/getQuarantineConfig` for rspec, to avoid repeated paginated calls and get all quarantined test IDs in one round trip Tested locally using a gem built with these changes: <details><summary>local rspec invocation output</summary> ``` bundle exec rspec Randomized with seed 51378 Main .always_true always passes .coin_flip Test failed, checking if it can be quarantined: `./spec/main_spec.rb:12` Test is not quarantined, continuing sometimes passes (FAILED - 1) .always_true Test failed, checking if it can be quarantined: `./spec/main_spec.rb:18` Test is quarantined, overriding exception: expected false got true always fails Failures: 1) Main.coin_flip sometimes passes Failure/Error: expect(Main.coin_flip).to be true expected true got false # ./spec/main_spec.rb:13:in `block (3 levels) in <top (required)>' # ./gems/rspec_trunk_flaky_tests/lib/trunk_spec_helper.rb:147:in `run' # ./gems/rspec_trunk_flaky_tests/lib/trunk_spec_helper.rb:117:in `run_with_trunk' Top 3 slowest examples (0.4614 seconds, 99.4% of total time): Main.coin_flip sometimes passes 0.45776 seconds ./spec/main_spec.rb:12 Main.always_true always fails 0.00338 seconds ./spec/main_spec.rb:18 Main.always_true always passes 0.00025 seconds ./spec/main_spec.rb:6 Finished in 0.46415 seconds (files took 0.04015 seconds to load) 3 examples, 1 failure Failed examples: rspec ./spec/main_spec.rb:12 # Main.coin_flip sometimes passes Randomized with seed 51378 Flaky tests report upload complete ``` </details>
1 parent 850b0d0 commit 1cbbb03

File tree

6 files changed

+114
-258
lines changed

6 files changed

+114
-258
lines changed

api/src/client.rs

Lines changed: 54 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ use std::sync::mpsc::Sender;
33

44
use constants::{DEFAULT_ORIGIN, TRUNK_PUBLIC_API_ADDRESS_ENV};
55
use display::message::DisplayMessage;
6-
use http::{header::HeaderMap, HeaderValue};
7-
use reqwest::{header, Client, Response, StatusCode};
6+
use http::{HeaderValue, header::HeaderMap};
7+
use reqwest::{Client, Response, StatusCode, header};
88
use serde::de::DeserializeOwned;
99
use tokio::fs;
1010

@@ -194,42 +194,6 @@ impl ApiClient {
194194
.await
195195
}
196196

197-
pub async fn list_quarantined_tests(
198-
&self,
199-
request: &message::ListQuarantinedTestsRequest,
200-
) -> anyhow::Result<message::ListQuarantinedTestsResponse> {
201-
CallApi {
202-
action: || async {
203-
let response = self
204-
.trunk_api_client
205-
.post(format!("{}{}/flaky-tests/list-quarantined-tests", self.api_host, self.version_path_prefix))
206-
.json(&request)
207-
.send()
208-
.await?;
209-
210-
let response = status_code_help(
211-
response,
212-
CheckUnauthorized::Check,
213-
CheckNotFound::DoNotCheck,
214-
|_| String::from("Failed to list quarantined tests."),
215-
&self.api_host,
216-
&self.org_url_slug,
217-
)?;
218-
219-
self.deserialize_response::<message::ListQuarantinedTestsResponse>(response).await
220-
},
221-
log_progress_message: |time_elapsed, _| {
222-
format!("Listing quarantined tests from Trunk services is taking longer than expected. It has taken {} seconds so far.", time_elapsed.as_secs())
223-
},
224-
report_slow_progress_message: |time_elapsed| {
225-
format!("Listing quarantined tests from Trunk services is taking longer than {} seconds", time_elapsed.as_secs())
226-
},
227-
render_sender: self.render_sender.clone(),
228-
}
229-
.call_api()
230-
.await
231-
}
232-
233197
pub async fn put_bundle_to_s3<U: AsRef<str>, B: AsRef<Path>>(
234198
&self,
235199
url: U,
@@ -450,8 +414,8 @@ fn does_not_add_settings_if_domain_absent() {
450414
#[cfg(test)]
451415
mod tests {
452416
use std::sync::{
453-
atomic::{AtomicUsize, Ordering},
454417
Arc,
418+
atomic::{AtomicUsize, Ordering},
455419
};
456420

457421
use axum::{http::StatusCode, response::Response};
@@ -488,21 +452,23 @@ mod tests {
488452
ApiClient::new(String::from("mock-token"), String::from("mock-org"), None).unwrap();
489453
api_client.api_host.clone_from(&state.host);
490454

491-
assert!(api_client
492-
.get_quarantining_config(&message::GetQuarantineConfigRequest {
493-
repo: context::repo::RepoUrlParts {
494-
host: String::from("host"),
495-
owner: String::from("owner"),
496-
name: String::from("name"),
497-
},
498-
org_url_slug: String::from("org_url_slug"),
499-
test_identifiers: vec![],
500-
remote_urls: vec![],
501-
})
502-
.await
503-
.unwrap_err()
504-
.to_string()
505-
.contains("501 Not Implemented"));
455+
assert!(
456+
api_client
457+
.get_quarantining_config(&message::GetQuarantineConfigRequest {
458+
repo: context::repo::RepoUrlParts {
459+
host: String::from("host"),
460+
owner: String::from("owner"),
461+
name: String::from("name"),
462+
},
463+
org_url_slug: String::from("org_url_slug"),
464+
test_identifiers: vec![],
465+
remote_urls: vec![],
466+
})
467+
.await
468+
.unwrap_err()
469+
.to_string()
470+
.contains("501 Not Implemented")
471+
);
506472
assert_eq!(CALL_COUNT.load(Ordering::Relaxed), 1);
507473
}
508474

@@ -532,21 +498,23 @@ mod tests {
532498
ApiClient::new(String::from("mock-token"), String::from("mock-org"), None).unwrap();
533499
api_client.api_host.clone_from(&state.host);
534500

535-
assert!(api_client
536-
.get_quarantining_config(&message::GetQuarantineConfigRequest {
537-
repo: context::repo::RepoUrlParts {
538-
host: String::from("api_host"),
539-
owner: String::from("owner"),
540-
name: String::from("name"),
541-
},
542-
remote_urls: vec![],
543-
org_url_slug: String::from("org_url_slug"),
544-
test_identifiers: vec![],
545-
})
546-
.await
547-
.unwrap_err()
548-
.to_string()
549-
.contains("500 Internal Server Error"));
501+
assert!(
502+
api_client
503+
.get_quarantining_config(&message::GetQuarantineConfigRequest {
504+
repo: context::repo::RepoUrlParts {
505+
host: String::from("api_host"),
506+
owner: String::from("owner"),
507+
name: String::from("name"),
508+
},
509+
remote_urls: vec![],
510+
org_url_slug: String::from("org_url_slug"),
511+
test_identifiers: vec![],
512+
})
513+
.await
514+
.unwrap_err()
515+
.to_string()
516+
.contains("500 Internal Server Error")
517+
);
550518
assert_eq!(CALL_COUNT.load(Ordering::Relaxed), 6);
551519
}
552520

@@ -571,20 +539,22 @@ mod tests {
571539
ApiClient::new(String::from("mock-token"), String::from("mock-org"), None).unwrap();
572540
api_client.api_host.clone_from(&state.host);
573541

574-
assert!(api_client
575-
.get_quarantining_config(&message::GetQuarantineConfigRequest {
576-
repo: context::repo::RepoUrlParts {
577-
host: String::from("api_host"),
578-
owner: String::from("owner"),
579-
name: String::from("name"),
580-
},
581-
remote_urls: vec![],
582-
org_url_slug: String::from("org_url_slug"),
583-
test_identifiers: vec![],
584-
})
585-
.await
586-
.unwrap_err()
587-
.to_string()
588-
.contains("404 Not Found"));
542+
assert!(
543+
api_client
544+
.get_quarantining_config(&message::GetQuarantineConfigRequest {
545+
repo: context::repo::RepoUrlParts {
546+
host: String::from("api_host"),
547+
owner: String::from("owner"),
548+
name: String::from("name"),
549+
},
550+
remote_urls: vec![],
551+
org_url_slug: String::from("org_url_slug"),
552+
test_identifiers: vec![],
553+
})
554+
.await
555+
.unwrap_err()
556+
.to_string()
557+
.contains("404 Not Found")
558+
);
589559
}
590560
}

api/src/message.rs

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -65,51 +65,3 @@ pub struct CreateBundleUploadIntentResponse {
6565
pub struct TelemetryUploadMetricsRequest {
6666
pub upload_metrics: proto::upload_metrics::trunk::UploadMetrics,
6767
}
68-
69-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
70-
pub struct PageQuery {
71-
pub page_size: i32,
72-
pub page_token: String,
73-
}
74-
75-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
76-
pub struct Page {
77-
pub total_rows: i32,
78-
pub total_pages: i32,
79-
pub next_page_token: String,
80-
pub prev_page_token: String,
81-
pub last_page_token: String,
82-
pub page_index: i32,
83-
}
84-
85-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
86-
pub struct ListQuarantinedTestsRequest {
87-
pub repo: RepoUrlParts,
88-
pub org_url_slug: String,
89-
pub page_query: PageQuery,
90-
}
91-
92-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
93-
pub enum QuarantineSetting {
94-
#[serde(rename = "ALWAYS_QUARANTINE")]
95-
AlwaysQuarantine,
96-
#[serde(rename = "AUTO_QUARANTINE")]
97-
AutoQuarantine,
98-
}
99-
100-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
101-
pub struct QuarantinedTest {
102-
pub name: String,
103-
pub parent: Option<String>,
104-
pub file: Option<String>,
105-
pub classname: Option<String>,
106-
pub status: String,
107-
pub quarantine_setting: QuarantineSetting,
108-
pub test_case_id: String,
109-
}
110-
111-
#[derive(Debug, Serialize, Clone, Deserialize, PartialEq)]
112-
pub struct ListQuarantinedTestsResponse {
113-
pub quarantined_tests: Vec<QuarantinedTest>,
114-
pub page: Page,
115-
}

cli-tests/src/upload.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ async fn upload_bundle() {
6565

6666
let quarantine_request = requests_iter.next().unwrap();
6767
let mut failure_count = 0;
68-
assert_matches!(quarantine_request, RequestPayload::GetQuarantineBulkTestStatus(req) => {
68+
assert_matches!(quarantine_request, RequestPayload::GetQuarantineConfig(req) => {
6969
assert_eq!(req.repo.host, "github.com");
7070
assert_eq!(req.repo.owner, "trunk-io");
7171
assert_eq!(req.repo.name, "analytics-cli");
@@ -1192,7 +1192,7 @@ async fn test_variant_propagation() {
11921192

11931193
assert_matches!(
11941194
requests_iter.next().unwrap(),
1195-
RequestPayload::GetQuarantineBulkTestStatus(_)
1195+
RequestPayload::GetQuarantineConfig(_)
11961196
);
11971197

11981198
assert_matches!(

test_report/src/report.rs

Lines changed: 29 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use chrono::prelude::*;
1313
use codeowners::CodeOwners;
1414
use context::repo::{BundleRepo, RepoUrlParts};
1515
#[cfg(feature = "ruby")]
16-
use magnus::{value::ReprValue, Module, Object};
16+
use magnus::{Module, Object, value::ReprValue};
1717
use prost_wkt_types::Timestamp;
1818
use proto::test_context::test_run::{
1919
CodeOwner, TestCaseRun, TestCaseRunStatus, TestReport as TestReportProto, TestResult,
@@ -280,7 +280,12 @@ impl MutTestReport {
280280
None,
281281
variant.clone(),
282282
);
283-
self.populate_quarantined_tests(&api_client, &bundle_repo.repo, org_url_slug);
283+
self.populate_quarantined_tests(
284+
&api_client,
285+
&bundle_repo.repo,
286+
bundle_repo.repo_url,
287+
org_url_slug,
288+
);
284289
if let Some(quarantined_tests) = self.0.borrow().quarantined_tests.as_ref() {
285290
return quarantined_tests.get(&test_identifier.id).is_some();
286291
}
@@ -297,47 +302,39 @@ impl MutTestReport {
297302
&self,
298303
api_client: &ApiClient,
299304
repo: &RepoUrlParts,
305+
repo_url: String,
300306
org_url_slug: String,
301307
) {
302308
if self.0.borrow().quarantined_tests.as_ref().is_some() {
303309
// already fetched
304310
return;
305311
}
306312
let mut quarantined_tests = HashMap::new();
307-
let mut request = message::ListQuarantinedTestsRequest {
308-
org_url_slug: org_url_slug.clone(),
309-
page_query: message::PageQuery {
310-
page_size: 100,
311-
page_token: String::new(),
312-
},
313+
let request = message::GetQuarantineConfigRequest {
314+
org_url_slug,
315+
test_identifiers: vec![],
316+
remote_urls: vec![repo_url],
313317
repo: repo.clone(),
314318
};
315-
loop {
316-
let response = tokio::runtime::Builder::new_multi_thread()
317-
.enable_all()
318-
.build()
319-
.unwrap()
320-
.block_on(api_client.list_quarantined_tests(&request));
321-
match response {
322-
Ok(response) => {
323-
for test in response.quarantined_tests.iter() {
324-
quarantined_tests.insert(test.test_case_id.clone(), true);
325-
}
326-
if response.page.next_page_token.is_empty() {
327-
break;
328-
}
329-
request.page_query.page_token = response.page.next_page_token;
330-
}
331-
Err(err) => {
332-
tracing::warn!("Unable to fetch quarantined tests");
333-
tracing::error!(
334-
hidden_in_console = true,
335-
"Error fetching quarantined tests: {:?}",
336-
err
337-
);
338-
break;
319+
let response = tokio::runtime::Builder::new_multi_thread()
320+
.enable_all()
321+
.build()
322+
.unwrap()
323+
.block_on(api_client.get_quarantining_config(&request));
324+
match response {
325+
Ok(response) => {
326+
for quarantined_test_id in response.quarantined_tests.iter() {
327+
quarantined_tests.insert(quarantined_test_id.clone(), true);
339328
}
340329
}
330+
Err(err) => {
331+
tracing::warn!("Unable to fetch quarantined tests");
332+
tracing::error!(
333+
hidden_in_console = true,
334+
"Error fetching quarantined tests: {:?}",
335+
err
336+
);
337+
}
341338
}
342339
self.0.borrow_mut().quarantined_tests = Some(quarantined_tests);
343340
}

0 commit comments

Comments
 (0)