Add integration tests for basic interactions with most of the commands #1181
Conversation
kamilogorek
commented
Apr 8, 2022
kamilogorek
commented
kamilogorek
force-pushed
the
more-integration-tests
branch
from
0e952bd to
16dd004
Compare
here/api__0.json
Outdated
| @@ -0,0 +1 @@ | |||
| {"version":"0","auth":null,"user":null} | |||
There was a problem hiding this comment.
where is this being generated?
There was a problem hiding this comment.
Whoops. Removed.
src/api.rs
Outdated
| debug!("body: {}", body); | ||
|
|
||
| // Internal helper for making it easier to write integration tests. | ||
| if let Ok(dir) = env::var("SENTRY_DUMP") { |
There was a problem hiding this comment.
do we need to document SENTRY_DUMP? Also maybe we make the env variable more specific to sentry cli?
There was a problem hiding this comment.
I'd not do that tbh. It's only a helper for me, to make writing tests for all existing commands easier. I might remove it in the future entirely. Added better comment, renamed it and handled errors.
src/commands/login.rs
Outdated
| @@ -37,6 +39,12 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { | |||
| .unwrap_or("<unknown>") | |||
| ); | |||
|
|
|||
| // It's not currently possible to easily mock I/O with `trycmd`, | |||
| // but verifying that `execute` is not panicking, is good enough for now. | |||
| if env::var("TEST").is_ok() { | |||
There was a problem hiding this comment.
TEST is also pretty generalized, maybe we choose a more specific env variable name?
We should also debug log so it's clear why we are skipping the rest of execute.
This also applies to the rest of the usages of env::var("TEST")
src/api.rs
Outdated
| @@ -1864,6 +1876,26 @@ fn log_headers(is_response: bool, data: &[u8]) { | |||
| } | |||
| } | |||
|
|
|||
| fn dump_response(mut dir: String, url: &str, body: String) { | |||
There was a problem hiding this comment.
shouldn't this return an error vs. emitting a panic that bubbles up?
There was a problem hiding this comment.
Updated. (handle error only, not panic, as this is not part of the user interface)
| #[cfg(not(windows))] | ||
| #[test] | ||
| fn command_debug_files_upload_help() { | ||
| register_test("debug_files/debug_files-upload-help.trycmd"); |
There was a problem hiding this comment.
windows CI is notoriously slow, esp for I/O operations. We ran into this when trying to run the Node CI in JS on windows: getsentry/sentry-javascript#4616. Maybe we leave a TODO comment on the command as a future performance optimization?
Even opening a GH issue with the list of these disabled tests with help wanted might be nice, maybe somebody else can help figure it out for us!
