Add ability to wrap test execution + check for quarantining #31

gnalh · 2024-03-20T01:09:16Z

In order to be able to quarantine tests we need to wrap the test execution and inspect the results of the test. If a test fails and that test is quarantined then we will override the exit code, otherwise we will return the original exit code.

Example:

cargo run test --junit-paths=py/*.xml --org-url-slug trunk-staging-org --token=[REDACTED] --dry-run -- pytest py/test.py --junitxml=py/test.xml

trunk-staging-io · 2024-03-20T06:03:56Z

💊 0 quarantined ✅ 10 passed 🕐 10 new ⋅ (learn more)

_{docs ⋅ learn more about trunk.io}

gnalh · 2024-03-20T16:27:46Z

.github/workflows/pull_request.yml

@@ -45,7 +45,7 @@ jobs:

      - name: Upload results using action from ${{ matrix.target }}
        env:
-          TRUNK_API_ADDRESS: https://api.trunk-staging.io/v1/metrics/createBundleUpload
+          TRUNK_API_ADDRESS: https://api.trunk-staging.io


We'll need to update our own repo to match once this is live.

should we still call this TRUNK_API_ADDRESS? maybe TRUNK_API_ORIGIN would be more appropriate?

TRUNK_API_ADDRESS is a pattern we use elsewhere.

src/constants.rs

src/main.rs

zaycev · 2024-03-20T18:26:46Z

src/main.rs

-    if let Err(e) = run(cli).await {
-        log::error!("Error: {:?}", e);
-        std::process::exit(exitcode::SOFTWARE);
+    match run(cli).await {


Maybe I'm missing something but if we already have result which can be either success or failure, why do we also need to return exit code? What would Ok(EXIT_FAILURE) mean? Does it mean we successfully captured failure of a test run and we want to propagate it to the caller of cli?

We need to pipe the original exit code up so that we surface the expected results to the caller. Since the plan is to wrap test executions, we don't want to hide it if the results weren't quarantined.

src/main.rs

zaycev · 2024-03-20T18:34:39Z

src/runner.rs

+                let file = std::fs::File::open(&file.original_path)?;
+                let reader = std::io::BufReader::new(file);
+                let junitxml = junit_parser::from_reader(reader)?;
+                for suite in junitxml.suites {


Feel free to do it in the next PR, testing parsing of XMLs is totally something we should test before rolling this out.

Running into issues getting tests running on my machine. Will address in a follow-up.

zaycev · 2024-03-20T18:35:43Z

src/runner.rs

+    command: &String,
+    args: Vec<&String>,
+    output_paths: Vec<&String>,
+) -> anyhow::Result<RunResult> {


If this function fails to process output of the command, what will happen? Do we ever want to override exit code of the child command?

We should only override the exit code if the only failures parsed were quarantined.

zaycev · 2024-03-20T18:36:39Z

Overall – LGTM.

gnalh · 2024-03-20T18:38:28Z

src/main.rs

 // Tokio-retry uses base ^ retry * factor formula.
 // This will give us 8ms, 64ms, 512ms, 4096ms, 32768ms
 const RETRY_BASE_MS: u64 = 8;
 const RETRY_FACTOR: u64 = 1;
-const RETRY_COUNT: usize = 5;
+const RETRY_COUNT: usize = 3;


5 was causing very long delays when there were issues with the api.

I've brought this up with Vlad before and he suggested keeping it at 5 because this gives the backend enough time to auto-resolve issues, expensive queries to finish, etc. Not sure if this is still a concern though cc @zaycev

The delay is very long with 5 (10+ seconds). I think we need a lower number.

Yeah, I'd mentioned that to Vlad as well. Will let him comment on this

5 was causing very long delays when there were issues with the api.

That was the original point for adding this delay - wait enough for transient issue to resolve. What are the down sites of it?

It feels broken when nothing happens for 20 seconds. If every api call has this retry logic then we end up wasting a lot of time.

src/clients.rs

riya-n · 2024-03-20T19:38:08Z

.github/workflows/pull_request.yml

@@ -45,7 +45,7 @@ jobs:

      - name: Upload results using action from ${{ matrix.target }}
        env:
-          TRUNK_API_ADDRESS: https://api.trunk-staging.io/v1/metrics/createBundleUpload
+          TRUNK_API_ADDRESS: https://api.trunk-staging.io


should we still call this TRUNK_API_ADDRESS? maybe TRUNK_API_ORIGIN would be more appropriate?

riya-n · 2024-03-20T19:41:23Z

src/main.rs

 // Tokio-retry uses base ^ retry * factor formula.
 // This will give us 8ms, 64ms, 512ms, 4096ms, 32768ms
 const RETRY_BASE_MS: u64 = 8;
 const RETRY_FACTOR: u64 = 1;
-const RETRY_COUNT: usize = 5;
+const RETRY_COUNT: usize = 3;


I've brought this up with Vlad before and he suggested keeping it at 5 because this gives the backend enough time to auto-resolve issues, expensive queries to finish, etc. Not sure if this is still a concern though cc @zaycev

src/main.rs

gnalh · 2024-03-20T19:57:02Z

This is ready for a re-review.

zaycev · 2024-03-21T18:56:04Z

LGTM

gnalh force-pushed the gabe/analytics-wrapper branch from f62f6b6 to 11bb9ff Compare March 20, 2024 01:11

init

f0aa1fc

gnalh force-pushed the gabe/analytics-wrapper branch from 11bb9ff to f0aa1fc Compare March 20, 2024 03:03

gnalh force-pushed the gabe/analytics-wrapper branch 2 times, most recently from b7d232c to 36259d8 Compare March 20, 2024 16:23

init

5db8281

gnalh force-pushed the gabe/analytics-wrapper branch from 36259d8 to 5db8281 Compare March 20, 2024 16:26

gnalh commented Mar 20, 2024

View reviewed changes

gnalh requested review from zaycev and riya-n March 20, 2024 16:31

gnalh marked this pull request as ready for review March 20, 2024 16:31

gnalh marked this pull request as draft March 20, 2024 16:31

add test command

fa45209

gnalh marked this pull request as ready for review March 20, 2024 18:10

gnalh changed the title ~~Add ability to wrap test execution~~ Add ability to wrap test execution + check for quarantining Mar 20, 2024

zaycev reviewed Mar 20, 2024

View reviewed changes

src/main.rs Show resolved Hide resolved

zaycev reviewed Mar 20, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

zaycev reviewed Mar 20, 2024

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

zaycev reviewed Mar 20, 2024

View reviewed changes

better handle thrown errors

3194c7e

gnalh commented Mar 20, 2024

View reviewed changes

drop ref

7df251f

gnalh requested a review from zaycev March 20, 2024 18:45

move trunk_api_address to constants

2526379

riya-n reviewed Mar 20, 2024

View reviewed changes

riya-n self-requested a review March 20, 2024 19:49

comments

d3bd9ca

riya-n approved these changes Mar 20, 2024

View reviewed changes

gnalh added 4 commits March 20, 2024 23:28

Merge branch 'main' into gabe/analytics-wrapper

4122ab2

fmt

65cadcb

Merge branch 'main' into gabe/analytics-wrapper

1c1a6d8

check

6842e62

zaycev approved these changes Mar 21, 2024

View reviewed changes

logging + small bug fix

9a0a088

gnalh merged commit ffc06f3 into main Mar 22, 2024
5 checks passed

gnalh mentioned this pull request Mar 22, 2024

Update analytics uploader versioning trunk-io/analytics-uploader#17

Merged

riya-n mentioned this pull request Jul 9, 2024

Don't fail on failed uploads #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to wrap test execution + check for quarantining #31

Add ability to wrap test execution + check for quarantining #31

gnalh commented Mar 20, 2024 •

edited

Loading

trunk-staging-io bot commented Mar 20, 2024 •

edited

Loading

gnalh Mar 20, 2024

riya-n Mar 20, 2024

gnalh Mar 20, 2024 •

edited

Loading

zaycev Mar 20, 2024

gnalh Mar 20, 2024 •

edited

Loading

zaycev Mar 20, 2024 •

edited

Loading

gnalh Mar 20, 2024

zaycev Mar 20, 2024

gnalh Mar 20, 2024

zaycev commented Mar 20, 2024

gnalh Mar 20, 2024

riya-n Mar 20, 2024

gnalh Mar 20, 2024

riya-n Mar 20, 2024

zaycev Mar 21, 2024

gnalh Mar 21, 2024 •

edited

Loading

riya-n Mar 20, 2024

riya-n Mar 20, 2024

gnalh commented Mar 20, 2024

zaycev commented Mar 21, 2024

Add ability to wrap test execution + check for quarantining #31

Add ability to wrap test execution + check for quarantining #31

Conversation

gnalh commented Mar 20, 2024 • edited Loading

trunk-staging-io bot commented Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnalh Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnalh Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

zaycev Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaycev commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnalh Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnalh commented Mar 20, 2024

zaycev commented Mar 21, 2024

gnalh commented Mar 20, 2024 •

edited

Loading

trunk-staging-io bot commented Mar 20, 2024 •

edited

Loading

gnalh Mar 20, 2024 •

edited

Loading

gnalh Mar 20, 2024 •

edited

Loading

zaycev Mar 20, 2024 •

edited

Loading

gnalh Mar 21, 2024 •

edited

Loading