test(gmail): add unit tests for +triage argument parsing#445
test(gmail): add unit tests for +triage argument parsing#445anshul-garg27 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
triage.rs was the only helper file with zero test coverage. Add tests for: default max (20), explicit max, non-numeric max fallback, custom query, labels flag, and output format selection.
googleworkspace-bot
added
the
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🦋 Changeset detectedLatest commit: 84cdd27 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the gmail +triage argument parsing. However, the current implementation duplicates the parsing logic from the production code within the tests, which makes them brittle. I've provided a suggestion to refactor the tests to centralize this logic, making them cleaner and more maintainable. This also highlights an opportunity to make the production code more testable by extracting the argument parsing logic.
| #[cfg(test)] | ||
| mod tests { | ||
| use clap::{Arg, ArgAction, Command}; | ||
|
|
||
| /// Build a clap command matching the +triage definition so we can | ||
| /// unit-test argument parsing without needing a live GmailHelper. | ||
| fn triage_cmd() -> Command { | ||
| Command::new("triage") | ||
| .arg( | ||
| Arg::new("max") | ||
| .long("max") | ||
| .default_value("20") | ||
| .value_name("N"), | ||
| ) | ||
| .arg(Arg::new("query").long("query").value_name("QUERY")) | ||
| .arg( | ||
| Arg::new("labels") | ||
| .long("labels") | ||
| .action(ArgAction::SetTrue), | ||
| ) | ||
| .arg(Arg::new("format").long("format").value_name("FMT")) | ||
| } | ||
|
|
||
| #[test] | ||
| fn defaults_max_to_20_and_query_to_unread() { | ||
| let m = triage_cmd().try_get_matches_from(["triage"]).unwrap(); | ||
| let max: u32 = m | ||
| .get_one::<String>("max") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(20); | ||
| let query = m | ||
| .get_one::<String>("query") | ||
| .map(|s| s.as_str()) | ||
| .unwrap_or("is:unread"); | ||
| assert_eq!(max, 20); | ||
| assert_eq!(query, "is:unread"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn explicit_max_overrides_default() { | ||
| let m = triage_cmd() | ||
| .try_get_matches_from(["triage", "--max", "5"]) | ||
| .unwrap(); | ||
| let max: u32 = m | ||
| .get_one::<String>("max") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(20); | ||
| assert_eq!(max, 5); | ||
| } | ||
|
|
||
| #[test] | ||
| fn non_numeric_max_falls_back_to_20() { | ||
| let m = triage_cmd() | ||
| .try_get_matches_from(["triage", "--max", "abc"]) | ||
| .unwrap(); | ||
| let max: u32 = m | ||
| .get_one::<String>("max") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(20); | ||
| assert_eq!(max, 20); | ||
| } | ||
|
|
||
| #[test] | ||
| fn custom_query_overrides_default() { | ||
| let m = triage_cmd() | ||
| .try_get_matches_from(["triage", "--query", "from:boss"]) | ||
| .unwrap(); | ||
| let query = m | ||
| .get_one::<String>("query") | ||
| .map(|s| s.as_str()) | ||
| .unwrap_or("is:unread"); | ||
| assert_eq!(query, "from:boss"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn labels_flag_defaults_to_false() { | ||
| let m = triage_cmd().try_get_matches_from(["triage"]).unwrap(); | ||
| assert!(!m.get_flag("labels")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn labels_flag_set_when_passed() { | ||
| let m = triage_cmd() | ||
| .try_get_matches_from(["triage", "--labels"]) | ||
| .unwrap(); | ||
| assert!(m.get_flag("labels")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn format_defaults_to_table_when_absent() { | ||
| let m = triage_cmd().try_get_matches_from(["triage"]).unwrap(); | ||
| let fmt = m | ||
| .get_one::<String>("format") | ||
| .map(|s| crate::formatter::OutputFormat::from_str(s)) | ||
| .unwrap_or(crate::formatter::OutputFormat::Table); | ||
| assert!(matches!(fmt, crate::formatter::OutputFormat::Table)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn format_json_when_specified() { | ||
| let m = triage_cmd() | ||
| .try_get_matches_from(["triage", "--format", "json"]) | ||
| .unwrap(); | ||
| let fmt = m | ||
| .get_one::<String>("format") | ||
| .map(|s| crate::formatter::OutputFormat::from_str(s)) | ||
| .unwrap_or(crate::formatter::OutputFormat::Table); | ||
| assert!(matches!(fmt, crate::formatter::OutputFormat::Json)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current test implementation duplicates the argument parsing logic from handle_triage within each test case. This is a violation of the Don't Repeat Yourself (DRY) principle and makes the tests brittle. If the parsing logic in handle_triage were to change, these tests would need to be updated in lockstep, and might not catch regressions.
To improve this, you can centralize the parsing logic into a helper function within the test module. This makes the tests cleaner and easier to maintain.
For a more robust long-term solution, consider extracting the argument parsing logic out of handle_triage entirely, so that both the production code and test code can share it. However, the following suggestion keeps the changes confined to the new test module.
#[cfg(test)]
mod tests {
use clap::{Arg, ArgAction, ArgMatches, Command};
// A struct to hold parsed arguments, mirroring the logic in `handle_triage`.
#[derive(Debug, PartialEq)]
struct TriageArgs<'a> {
max: u32,
query: &'a str,
show_labels: bool,
output_format: crate::formatter::OutputFormat,
}
// Helper to parse args, centralizing the logic copied from `handle_triage`.
fn parse_triage_args(m: &ArgMatches) -> TriageArgs {
let max = m
.get_one::<String>("max")
.and_then(|s| s.parse().ok())
.unwrap_or(20);
let query = m
.get_one::<String>("query")
.map(|s| s.as_str())
.unwrap_or("is:unread");
let show_labels = m.get_flag("labels");
let output_format = m
.get_one::<String>("format")
.map(|s| crate::formatter::OutputFormat::from_str(s))
.unwrap_or(crate::formatter::OutputFormat::Table);
TriageArgs {
max,
query,
show_labels,
output_format,
}
}
/// Build a clap command matching the +triage definition so we can
/// unit-test argument parsing without needing a live GmailHelper.
fn triage_cmd() -> Command {
Command::new("triage")
.arg(
Arg::new("max")
.long("max")
.default_value("20")
.value_name("N"),
)
.arg(Arg::new("query").long("query").value_name("QUERY"))
.arg(
Arg::new("labels")
.long("labels")
.action(ArgAction::SetTrue),
)
.arg(Arg::new("format").long("format").value_name("FMT"))
}
#[test]
fn defaults_max_to_20_and_query_to_unread() {
let m = triage_cmd().try_get_matches_from(["triage"]).unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.max, 20);
assert_eq!(args.query, "is:unread");
}
#[test]
fn explicit_max_overrides_default() {
let m = triage_cmd()
.try_get_matches_from(["triage", "--max", "5"])
.unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.max, 5);
}
#[test]
fn non_numeric_max_falls_back_to_20() {
let m = triage_cmd()
.try_get_matches_from(["triage", "--max", "abc"])
.unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.max, 20);
}
#[test]
fn custom_query_overrides_default() {
let m = triage_cmd()
.try_get_matches_from(["triage", "--query", "from:boss"])
.unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.query, "from:boss");
}
#[test]
fn labels_flag_defaults_to_false() {
let m = triage_cmd().try_get_matches_from(["triage"]).unwrap();
let args = parse_triage_args(&m);
assert!(!args.show_labels);
}
#[test]
fn labels_flag_set_when_passed() {
let m = triage_cmd()
.try_get_matches_from(["triage", "--labels"])
.unwrap();
let args = parse_triage_args(&m);
assert!(args.show_labels);
}
#[test]
fn format_defaults_to_table_when_absent() {
let m = triage_cmd().try_get_matches_from(["triage"]).unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.output_format, crate::formatter::OutputFormat::Table);
}
#[test]
fn format_json_when_specified() {
let m = triage_cmd()
.try_get_matches_from(["triage", "--format", "json"])
.unwrap();
let args = parse_triage_args(&m);
assert_eq!(args.output_format, crate::formatter::OutputFormat::Json);
}
}2 participants
Summary
gmail +triageargument parsing — previously the only helper file with zero test coverage--max,--queryoverride,--labelsflag, and--formatselectionDetails
src/helpers/gmail/triage.rshad no#[cfg(test)]module. While the core logic involves network calls (Gmail API), the argument parsing and format selection are purely deterministic and worth covering.Tests added
defaults_max_to_20_and_query_to_unread— verifies default valuesexplicit_max_overrides_default—--max 5parses correctlynon_numeric_max_falls_back_to_20—--max abcsilently falls backcustom_query_overrides_default—--query 'from:boss'workslabels_flag_defaults_to_false— default behaviorlabels_flag_set_when_passed—--labelssets the flagformat_defaults_to_table_when_absent— table is the defaultformat_json_when_specified—--format jsonswitches output