Improve error reporting, exit codes, output consistency, and documentation#132
Merged
Conversation
The `RequestError` variant previously displayed a static message with no details, making it impossible to diagnose failures such as connection refused or invalid URLs without additional tooling. Include the underlying `reqwest::Error` in the formatted message so the root cause is visible directly in the CLI output.
Add `CommandRunError` message formatting tests covering `RequestError`, `InvalidHeaderValue`, and `IncompatibleBody`. The `IncompatibleBody` test is intentionally failing until that variant is fixed next.
The catch-all error arm used `println!` with Debug format (`{:?}`),
sending raw Rust struct output to stdout. Switch to `eprintln!` with
Display format (`{}`) for consistent, human-readable error output on
stderr.
Add an integration test that verifies a connection error against an
unreachable host produces a useful message on stderr.
Replace the `_ => ExitCode::Usage` catch-all with explicit arms for every `CommandRunError` variant. Runtime and API errors map to `DataErr`, bad-invocation errors map to `Usage`, and `HealthCheckFailed` maps to `Unavailable`. The exhaustive match means the compiler will reject any future variant addition that is not explicitly handled, preventing silent misclassification. Add unit tests verifying that `RequestError`, `ClientError`, and `NotFound` all produce `ExitCode::DataErr`.
Add explicit lifetime annotation to `make_handler` in the unit test file to satisfy the `hidden_lifetime_parameters` lint under `-D warnings` (enforced in CI).
…re mode `CertificateFileCouldNotBeLoaded1` is a PEM parse failure; its message now says "could not be parsed as a PEM file". `CertificateFileCouldNotBeLoaded2` is an I/O read failure; its message now says "could not be read". Previously both variants had identical messages, making it impossible to tell from the error output whether the file was unreadable or contained invalid PEM data.
The quiet reporter is intended to suppress all output, but `finish_operation` was printing a completion summary. Make it consistent with all other methods by doing nothing. Add an integration test verifying that `--quiet` produces no stdout output when running `vhosts delete_multiple`.
Add a step-by-step guide for running the test suite locally using Docker, covering prerequisites, starting RabbitMQ, pre-configuring the node via `before_build.sh`, and running the tests. Explain why `NEXTEST_RETRIES=3` is recommended. Add a node teardown snippet. Reorganize the existing test structure and filter sections to follow the setup guide.
…NG.md Add `RUSTFLAGS="-D warnings"` to the nextest and clippy commands so agents run with the same strictness as CI. Fix `cargo clippy --all` to `cargo clippy --all-features`. Point the test node configuration section to CONTRIBUTING.md for the full Docker setup guide.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves error reporting and output consistency across several areas, and updates documentation for local development setup.
Error message improvements
Several
CommandRunErrorvariants were capturing useful context in their fields but not including it in the displayed message:RequestError: now includes the underlyingreqwest::Errordetail (e.g. "connection refused", "builder error")InvalidHeaderValue: now includes the underlying error detailIncompatibleBody: now includes the underlyingConversionErrordetailCertificateKeyMismatch: now includes both the cert and key file pathsCertificateFileCouldNotBeLoaded1andCertificateFileCouldNotBeLoaded2had identical messages despite representing different failure modes (PEM parse failure vs I/O read failure); they now have distinct, accurate messagesUnknownCommandTarget: fixed a missing closing'in the messageExit code mapping
report_pre_command_run_errorhad a_ => ExitCode::Usagecatch-all that incorrectly classified runtime errors (connection failures, API errors, 404s) as usage errors. The match is now exhaustive with explicit arms for every variant. This also means the compiler will reject any future variant addition that is not explicitly handled.Output consistency
health_check_resultwas usingprintln!with{:?}(Debug format). It now useseprintln!with{}(Display format), consistent with all other error reporting paths.QuietProgressReporter::finish_operationwas printing a completion summary despite the reporter being intended to suppress all output. It is now a no-op.Documentation
CONTRIBUTING.md: added a step-by-step Docker-based local test setup guide (start RabbitMQ, pre-configure the node, run the tests)AGENTS.md: addedRUSTFLAGS="-D warnings"to the nextest and clippy commands to match CI strictness; fixedcargo clippy --alltocargo clippy --all-featuresTests
All changes are covered by new unit and integration tests.