Skip to content

tests: replace many .unwrap()s with ?s using eyre #9111

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

Open
glehmann wants to merge 7 commits into
base: main
Choose a base branch
from
+4,817 −4,936
Open

tests: replace many .unwrap()s with ?s using eyre #9111

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
465c748
testutils: use eyre::Result as TestResult
glehmann Mar 14, 2026
b202110
testutils: use color-eyre for backtrace display
glehmann Mar 14, 2026
dbdc038
testutils: add custom backtrace frame filter to color-eyre
glehmann Mar 15, 2026
c9b89b3
testutils: enable backtrace capture by default (RUST_BACKTRACE=1) in …
glehmann Mar 15, 2026
80ee629
tests: `.block_on().unwrap()` with `.block_on()?` in tests using Test…
ilyagr Mar 14, 2026
e758f76
tests: Replace another ~600 `unwrap()`s in tests with `?`
ilyagr Mar 14, 2026
3b5a9a7
tests: Replace Result.unwrap with ? operator in test functions
glehmann Mar 18, 2026
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
Prev Previous commit
tests: Replace Result.unwrap with ? operator in test functions 
Mostly done with OpenCode and Claude Sonnet 4.6 with the following prompt,
and reviewed manually:

* Replace the Result.unwrap with the ? operator in test functions
* Add TestResult as return type in the test functions where you replace
  unwrap by the ? operator, and Ok(()) at the end.
* Do the replacement only in method with the `#[test]` decorator. You
  can't exclude files that are not in tests dirs, because some files have
  inline tests.
* Don't replace unwraps in closures.
* Don't replace Option.unwrap.
* Be careful to not change the insta snapshot references.
* You can identify all the Result.unwrap locations with this command:
  cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep / | grep Result | grep <filename>
* Run the tests each time you're done with a file.
* Begin with the file with the most Result.unwrap in them as shown by
  this command:
  cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | cut -d: -f1 | sort | uniq -c | sort -nr

Before this PR:

❯ cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | wc -l
3973

After this PR:

❯ cargo clippy --tests --message-format=short -- -W clippy::unwrap_used 2>&1 | grep Result | wc -l
1174
  • Loading branch information
@glehmann
glehmann committed Mar 18, 2026
commit 3b5a9a7c74523555f78b06167d658cd1d2ffa693
99 changes: 48 additions & 51 deletions cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,7 @@ mod tests {
use indoc::indoc;
use maplit::hashmap;
use test_case::test_case;
use testutils::TestResult;

use super::*;

Expand All @@ -1060,30 +1061,30 @@ mod tests {
}

#[test]
fn test_parse_value_or_bare_string() {
fn test_parse_value_or_bare_string() -> TestResult {
let parse = |s: &str| parse_value_or_bare_string(s);

// Value in TOML syntax
assert_eq!(parse("true").unwrap().as_bool(), Some(true));
assert_eq!(parse("42").unwrap().as_integer(), Some(42));
assert_eq!(parse("-1").unwrap().as_integer(), Some(-1));
assert_eq!(parse("'a'").unwrap().as_str(), Some("a"));
assert!(parse("[]").unwrap().is_array());
assert!(parse("{ a = 'b' }").unwrap().is_inline_table());
assert_eq!(parse("true")?.as_bool(), Some(true));
assert_eq!(parse("42")?.as_integer(), Some(42));
assert_eq!(parse("-1")?.as_integer(), Some(-1));
assert_eq!(parse("'a'")?.as_str(), Some("a"));
assert!(parse("[]")?.is_array());
assert!(parse("{ a = 'b' }")?.is_inline_table());

// Bare string
assert_eq!(parse("").unwrap().as_str(), Some(""));
assert_eq!(parse("John Doe").unwrap().as_str(), Some("John Doe"));
assert_eq!(parse("Doe, John").unwrap().as_str(), Some("Doe, John"));
assert_eq!(parse("It's okay").unwrap().as_str(), Some("It's okay"));
assert_eq!(parse("")?.as_str(), Some(""));
assert_eq!(parse("John Doe")?.as_str(), Some("John Doe"));
assert_eq!(parse("Doe, John")?.as_str(), Some("Doe, John"));
assert_eq!(parse("It's okay")?.as_str(), Some("It's okay"));
assert_eq!(
parse("<foo+bar@example.org>").unwrap().as_str(),
parse("<foo+bar@example.org>")?.as_str(),
Some("<foo+bar@example.org>")
);
assert_eq!(parse("#ff00aa").unwrap().as_str(), Some("#ff00aa"));
assert_eq!(parse("all()").unwrap().as_str(), Some("all()"));
assert_eq!(parse("glob:*.*").unwrap().as_str(), Some("glob:*.*"));
assert_eq!(parse("柔術").unwrap().as_str(), Some("柔術"));
assert_eq!(parse("#ff00aa")?.as_str(), Some("#ff00aa"));
assert_eq!(parse("all()")?.as_str(), Some("all()"));
assert_eq!(parse("glob:*.*")?.as_str(), Some("glob:*.*"));
assert_eq!(parse("柔術")?.as_str(), Some("柔術"));

// Error in TOML value
assert!(parse("'foo").is_err());
Expand All @@ -1093,6 +1094,7 @@ mod tests {
assert!(parse("\n { x").is_err());
assert!(parse(" x ] ").is_err());
assert!(parse("[table]\nkey = 'value'").is_err());
Ok(())
}

#[test]
Expand Down Expand Up @@ -1140,12 +1142,11 @@ mod tests {
}

#[test]
fn test_command_args() {
fn test_command_args() -> TestResult {
let mut config = StackedConfig::empty();
config.add_layer(
ConfigLayer::parse(
ConfigSource::User,
indoc! {"
config.add_layer(ConfigLayer::parse(
ConfigSource::User,
indoc! {"
empty_array = []
empty_string = ''
array = ['emacs', '-nw']
Expand All @@ -1154,19 +1155,17 @@ mod tests {
structured.env = { KEY1 = 'value1', KEY2 = 'value2' }
structured.command = ['emacs', '-nw']
"},
)
.unwrap(),
);
)?);

assert!(config.get::<CommandNameAndArgs>("empty_array").is_err());

let command_args: CommandNameAndArgs = config.get("empty_string").unwrap();
let command_args: CommandNameAndArgs = config.get("empty_string")?;
assert_eq!(command_args, CommandNameAndArgs::String("".to_owned()));
let (name, args) = command_args.split_name_and_args();
assert_eq!(name, "");
assert!(args.is_empty());

let command_args: CommandNameAndArgs = config.get("array").unwrap();
let command_args: CommandNameAndArgs = config.get("array")?;
assert_eq!(
command_args,
CommandNameAndArgs::Vec(NonEmptyCommandArgsVec(
Expand All @@ -1177,7 +1176,7 @@ mod tests {
assert_eq!(name, "emacs");
assert_eq!(args, ["-nw"].as_ref());

let command_args: CommandNameAndArgs = config.get("string").unwrap();
let command_args: CommandNameAndArgs = config.get("string")?;
assert_eq!(
command_args,
CommandNameAndArgs::String("emacs -nw".to_owned())
Expand All @@ -1186,7 +1185,7 @@ mod tests {
assert_eq!(name, "emacs");
assert_eq!(args, ["-nw"].as_ref());

let command_args: CommandNameAndArgs = config.get("string_quoted").unwrap();
let command_args: CommandNameAndArgs = config.get("string_quoted")?;
assert_eq!(
command_args,
CommandNameAndArgs::String("\"spaced path/to/emacs\" -nw".to_owned())
Expand All @@ -1195,7 +1194,7 @@ mod tests {
assert_eq!(name, "spaced path/to/emacs");
assert_eq!(args, ["-nw"].as_ref());

let command_args: CommandNameAndArgs = config.get("structured").unwrap();
let command_args: CommandNameAndArgs = config.get("structured")?;
assert_eq!(
command_args,
CommandNameAndArgs::Structured {
Expand All @@ -1209,6 +1208,7 @@ mod tests {
let (name, args) = command_args.split_name_and_args();
assert_eq!(name, "emacs");
assert_eq!(args, ["-nw"].as_ref());
Ok(())
}

#[test]
Expand All @@ -1218,20 +1218,14 @@ mod tests {
}

#[test]
fn test_resolved_config_values_single_key() {
fn test_resolved_config_values_single_key() -> TestResult {
let settings = insta_settings();
let _guard = settings.bind_to_scope();
let mut env_base_layer = ConfigLayer::empty(ConfigSource::EnvBase);
env_base_layer
.set_value("user.name", "base-user-name")
.unwrap();
env_base_layer
.set_value("user.email", "base@user.email")
.unwrap();
env_base_layer.set_value("user.name", "base-user-name")?;
env_base_layer.set_value("user.email", "base@user.email")?;
let mut repo_layer = ConfigLayer::empty(ConfigSource::Repo);
repo_layer
.set_value("user.email", "repo@user.email")
.unwrap();
repo_layer.set_value("user.email", "repo@user.email")?;
let mut config = StackedConfig::empty();
config.add_layer(env_base_layer);
config.add_layer(repo_layer);
Expand Down Expand Up @@ -1327,17 +1321,18 @@ mod tests {
]
"#
);
Ok(())
}

#[test]
fn test_resolved_config_values_filter_path() {
fn test_resolved_config_values_filter_path() -> TestResult {
let settings = insta_settings();
let _guard = settings.bind_to_scope();
let mut user_layer = ConfigLayer::empty(ConfigSource::User);
user_layer.set_value("test-table1.foo", "user-FOO").unwrap();
user_layer.set_value("test-table2.bar", "user-BAR").unwrap();
user_layer.set_value("test-table1.foo", "user-FOO")?;
user_layer.set_value("test-table2.bar", "user-BAR")?;
let mut repo_layer = ConfigLayer::empty(ConfigSource::Repo);
repo_layer.set_value("test-table1.bar", "repo-BAR").unwrap();
repo_layer.set_value("test-table1.bar", "repo-BAR")?;
let mut config = StackedConfig::empty();
config.add_layer(user_layer);
config.add_layer(repo_layer);
Expand Down Expand Up @@ -1404,10 +1399,11 @@ mod tests {
]
"#
);
Ok(())
}

#[test]
fn test_resolved_config_values_overridden() {
fn test_resolved_config_values_overridden() -> TestResult {
let list = |layers: &[&ConfigLayer], prefix: &str| -> String {
let mut config = StackedConfig::empty();
config.extend_layers(layers.iter().copied().cloned());
Expand All @@ -1426,15 +1422,15 @@ mod tests {
};

let mut layer0 = ConfigLayer::empty(ConfigSource::User);
layer0.set_value("a.b.e", "0.0").unwrap();
layer0.set_value("a.b.c.f", "0.1").unwrap();
layer0.set_value("a.b.d", "0.2").unwrap();
layer0.set_value("a.b.e", "0.0")?;
layer0.set_value("a.b.c.f", "0.1")?;
layer0.set_value("a.b.d", "0.2")?;
let mut layer1 = ConfigLayer::empty(ConfigSource::User);
layer1.set_value("a.b", "1.0").unwrap();
layer1.set_value("a.c", "1.1").unwrap();
layer1.set_value("a.b", "1.0")?;
layer1.set_value("a.c", "1.1")?;
let mut layer2 = ConfigLayer::empty(ConfigSource::User);
layer2.set_value("a.b.g", "2.0").unwrap();
layer2.set_value("a.b.d", "2.1").unwrap();
layer2.set_value("a.b.g", "2.0")?;
layer2.set_value("a.b.d", "2.1")?;

// a.b.* is shadowed by a.b
let layers = [&layer0, &layer1];
Expand Down Expand Up @@ -1510,6 +1506,7 @@ mod tests {
a.b.d = "2.1"
"#);
insta::assert_snapshot!(list(&layers, "a.b.c"), @r#"!a.b.c.f = "0.1""#);
Ok(())
}

struct TestCase {
Expand Down
Loading
Loading