force sentry to collect events with env dev for tests #122

dfrankland · 2024-10-09T16:14:40Z

I'm adding more Sentry events and I'm seeing these sometimes get sent using the prod env in tests 😬

This can happen when cargo test --release is used and Sentry defaults its environment based on the debug_assertions feature flag which is turned off/on by --release.

dfrankland · 2024-10-09T16:15:18Z

Cargo.toml

  "cli",
+  "cli-tests",


It's a fairly common pattern to split out a crate that's just for tests. For example, see tokio.

That's interesting. How does this typically work with code owners? I assume that both directories will be marked as owned by the same team / group of individuals?

Forgot to mention that splitting out test crates is necessary when enabling different features for tests.

I don't see people using code owners that often, so I'm not exactly sure. Assigning code owners can be as simple as

cli*/ @cli-team

or

cli/ @cli-team cli-*/ @cli-team

dfrankland · 2024-10-09T16:19:35Z

cli-tests/src/upload.rs

+lazy_static! {
+    static ref CARGO_MANIFEST_DIR: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    static ref CARGO_RUN: CargoRun = CargoBuild::new()
+        .bin("trunk-analytics-cli")
+        .target_dir(CARGO_MANIFEST_DIR.join("../target"))
+        .manifest_path(CARGO_MANIFEST_DIR.join("../cli/Cargo.toml"))
+        .features("force-sentry-env-dev")
+        .current_release()
+        .current_target()
+        .run()
+        .unwrap();
+}


escargot is a nice interface to build crates with features enabled or other things configured. It's recommended by assert_cmd.

dfrankland · 2024-10-09T16:21:16Z

cli/src/main.rs

-    let _guard = sentry::init((
-        SENTRY_DSN,
-        sentry::ClientOptions {
-            release: sentry::release_name!(),
-            ..Default::default()
-        },
-    ));
+    let mut options = sentry::ClientOptions::default();
+    options.release = sentry::release_name!();
+
+    #[cfg(feature = "force-sentry-env-dev")]
+    {
+        options.environment = Some("development".into())
+    }
+
+    let _guard = sentry::init((SENTRY_DSN, options));


environment defaults to "production" or "development" based on the debug_assertions feature flag. Our new feature force-sentry-env-dev forces it to "development" for tests.

trunk-staging-io · 2024-10-09T16:44:13Z

362 tests were run on d541df8f. ✅ 362 Passed. View Full Report ↗︎

_settings

gnalh · 2024-10-09T16:49:31Z

Cargo.toml

  "cli",
+  "cli-tests",


That's interesting. How does this typically work with code owners? I assume that both directories will be marked as owned by the same team / group of individuals?

gnalh · 2024-10-09T16:51:41Z

cli-tests/src/upload.rs

@@ -127,7 +144,7 @@ async fn upload_bundle() {
    assert_eq!(bundle_meta.envs.get("CI"), Some(&String::from("1")));
    let time_since_upload = chrono::Utc::now()
        - chrono::DateTime::from_timestamp(bundle_meta.upload_time_epoch as i64, 0).unwrap();
-    assert_eq!(time_since_upload.num_minutes(), 0);
+    assert!(time_since_upload.num_minutes() <= 5);


Nit: This seems to be a fairly popular crate that has an explicit assert_lt
https://docs.rs/more-asserts/latest/more_asserts/#

I personally prefer being explicit about expectations in tests since it helps with debugging.

No problem. I've been trying to keep things minimal until needed, but I have no issue with adding that. Thanks for doing the research 👍

gnalh · 2024-10-09T16:51:48Z

cli-tests/src/upload.rs

@@ -155,7 +172,7 @@ async fn upload_bundle() {
    );
    let time_since_junit_modified = chrono::Utc::now()
        - chrono::DateTime::from_timestamp_nanos(bundled_file.last_modified_epoch_ns as i64);
-    assert_eq!(time_since_junit_modified.num_minutes(), 0);
+    assert!(time_since_junit_modified.num_minutes() <= 5);


Similar comment to the above.

max-trunk

LG! couple questions for my Rust learning

max-trunk · 2024-10-09T16:59:02Z

cli-tests/src/upload.rs

+        .bin("trunk-analytics-cli")
+        .target_dir(CARGO_MANIFEST_DIR.join("../target"))
+        .manifest_path(CARGO_MANIFEST_DIR.join("../cli/Cargo.toml"))
+        .features("force-sentry-env-dev")


My guess is that every new test file in this crate that wants to invoke the CLI will need to do this? Not that it's a huge lift to copy it into new test files - but is there a Rust-y way to do this once and leverage it across modules?

Also - does enabling the force-sentry-env-dev feature in the dev dependencies not take care of this https://github.com/trunk-io/analytics-cli/pull/122/files#diff-48118bb96b547e6c79e43924f9db25f71065d526d458312f2c673458370dd4e3R19?

If by modules, you mean modules within cli-tests/src then we just need to add a pub to this static ref and reuse it in within the crate.

If you are thinking about how one might reuse this across crates in the workspace, then we could move this to test_utils with pub and reuse that here and elsewhere.

Enabling force-sentry-env-dev for the dependency only enables it for the lib.rs, not for the main.rs. Here we are using escargot to build the main.rs binary with the force-sentry-env-dev feature enabled.

If by modules, you mean modules within cli-tests/src then we just need to add a pub to this static ref and reuse it in within the crate.

Yep! this is what I meant. Thanks for the info 🫡

dfrankland requested review from mmatheson, gnalh and max-trunk October 9, 2024 16:14

dfrankland commented Oct 9, 2024

View reviewed changes

dfrankland force-pushed the dylan/force-sentry-env-dev branch from 2b5a31a to cf7460c Compare October 9, 2024 16:44

gnalh approved these changes Oct 9, 2024

View reviewed changes

max-trunk approved these changes Oct 9, 2024

View reviewed changes

dfrankland added 4 commits October 9, 2024 16:49

force sentry to collect events with env dev for tests

888d921

up time limit for upload test

3a0b504

add more_asserts

d96b963

Update Cargo.lock

c049a37

dfrankland force-pushed the dylan/force-sentry-env-dev branch from ccd775c to c049a37 Compare October 9, 2024 23:52

dfrankland merged commit f24b648 into main Oct 10, 2024
11 checks passed

dfrankland deleted the dylan/force-sentry-env-dev branch October 10, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

force sentry to collect events with env dev for tests #122

force sentry to collect events with env dev for tests #122

dfrankland commented Oct 9, 2024 •

edited

Loading

dfrankland Oct 9, 2024

gnalh Oct 9, 2024

dfrankland Oct 9, 2024

dfrankland Oct 9, 2024

dfrankland Oct 9, 2024

trunk-staging-io bot commented Oct 9, 2024 •

edited

Loading

gnalh Oct 9, 2024

gnalh Oct 9, 2024

dfrankland Oct 9, 2024

gnalh Oct 9, 2024

max-trunk left a comment

max-trunk Oct 9, 2024

dfrankland Oct 9, 2024

max-trunk Oct 9, 2024

force sentry to collect events with env dev for tests #122

force sentry to collect events with env dev for tests #122

Conversation

dfrankland commented Oct 9, 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

Choose a reason for hiding this comment

trunk-staging-io bot commented Oct 9, 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

max-trunk left a 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

dfrankland commented Oct 9, 2024 •

edited

Loading

trunk-staging-io bot commented Oct 9, 2024 •

edited

Loading