From ebe82c8c24da4683dd088583917078f510bc0186 Mon Sep 17 00:00:00 2001 From: Matthias Date: Wed, 6 Nov 2024 23:41:05 +0100 Subject: [PATCH] fix: Improve output formatting With the last lychee release, we simplified the status output for links. While this reduced the visual noise, it also accidentally caused the source of errors to not be printed anymore. This change brings back the additional error information as part of the final report output. Furthermore, it shows the error information in the progress output if verbose mode is activated. Fixes #1487 --- fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md | 10 ++--- fixtures/TEST_INVALID_URLS.html | 27 ++++++++++++ lychee-bin/src/formatters/response/plain.rs | 2 +- lychee-bin/src/formatters/stats/compact.rs | 3 +- lychee-bin/tests/cli.rs | 46 ++++++++++++++++++++- lychee-lib/src/types/response.rs | 19 +++++---- lychee-lib/src/types/status.rs | 2 +- 7 files changed, 92 insertions(+), 17 deletions(-) create mode 100644 fixtures/TEST_INVALID_URLS.html diff --git a/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md b/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md index 81e0535c5d..253fb9dcdf 100644 --- a/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md +++ b/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md @@ -1,8 +1,8 @@ -# Test detailed JSON output error +# Test Detailed JSON Output Error -This file is used to test if the error details are parsed properly in the json +This file is used to test if the error details are parsed properly in the JSON format. -[The website](https://expired.badssl.com/) produce SSL expired certificate -error. Such network error has no status code but it can be identified by error -status details. +[The website](https://expired.badssl.com/) produces an SSL expired certificate +error. Such a network error has no status code, but it can be identified by +error status details. diff --git a/fixtures/TEST_INVALID_URLS.html b/fixtures/TEST_INVALID_URLS.html new file mode 100644 index 0000000000..974082a61e --- /dev/null +++ b/fixtures/TEST_INVALID_URLS.html @@ -0,0 +1,27 @@ + + + + + + Invalid URLs + + + + + diff --git a/lychee-bin/src/formatters/response/plain.rs b/lychee-bin/src/formatters/response/plain.rs index 3cc5f7a459..c15ac42098 100644 --- a/lychee-bin/src/formatters/response/plain.rs +++ b/lychee-bin/src/formatters/response/plain.rs @@ -61,7 +61,7 @@ mod plain_tests { let body = mock_response_body(Status::Excluded, "https://example.com/not-checked"); assert_eq!( formatter.format_response(&body), - "[EXCLUDED] https://example.com/not-checked | Excluded" + "[EXCLUDED] https://example.com/not-checked" ); } diff --git a/lychee-bin/src/formatters/stats/compact.rs b/lychee-bin/src/formatters/stats/compact.rs index 86969a36d2..087acbc3a6 100644 --- a/lychee-bin/src/formatters/stats/compact.rs +++ b/lychee-bin/src/formatters/stats/compact.rs @@ -42,8 +42,7 @@ impl Display for CompactResponseStats { for response in responses { writeln!( f, - "[{}] {}", - response.status.code_as_string(), + "{}", response_formatter.format_detailed_response(response) )?; } diff --git a/lychee-bin/tests/cli.rs b/lychee-bin/tests/cli.rs index 1188f9af81..5f418c19cb 100644 --- a/lychee-bin/tests/cli.rs +++ b/lychee-bin/tests/cli.rs @@ -111,6 +111,50 @@ mod cli { }}; } + /// Test that the default report output format (compact) and mode (color) + /// prints the failed URLs as well as their status codes on error. Make + /// sure that the status code only occurs once. + #[test] + fn test_compact_output_format_contains_status() -> Result<()> { + let test_path = fixtures_path().join("TEST_INVALID_URLS.html"); + + let mut cmd = main_command(); + cmd.arg("--format") + .arg("compact") + .arg("--mode") + .arg("color") + .arg(test_path) + .env("FORCE_COLOR", "1") + .assert() + .failure() + .code(2); + + let output = cmd.output()?; + + // Check that the output contains the status code (once) and the URL + let output_str = String::from_utf8_lossy(&output.stdout); + + // The expected output is as follows: + // "Find details below." + // [EMPTY LINE] + // [path/to/file]: + // [400] https://httpbin.org/status/404 + // [500] https://httpbin.org/status/500 + // [502] https://httpbin.org/status/502 + // (the order of the URLs may vary) + + // Check that the output contains the file path + assert!(output_str.contains("TEST_INVALID_URLS.html")); + + let re = Regex::new(r"\s{5}\[\d{3}\] https://httpbin\.org/status/\d{3}").unwrap(); + let matches: Vec<&str> = re.find_iter(&output_str).map(|m| m.as_str()).collect(); + + // Check that the status code occurs only once + assert_eq!(matches.len(), 3); + + Ok(()) + } + /// JSON-formatted output should always be valid JSON. /// Additional hints and error messages should be printed to `stderr`. /// See https://github.com/lycheeverse/lychee/issues/1355 @@ -996,7 +1040,7 @@ mod cli { .stderr(contains(format!( "[IGNORED] {unsupported_url} | Unsupported: Error creating request client" ))) - .stderr(contains(format!("[EXCLUDED] {excluded_url} | Excluded\n"))); + .stderr(contains(format!("[EXCLUDED] {excluded_url}\n"))); // The cache file should be empty, because the only checked URL is // unsupported and we don't want to cache that. It might be supported in diff --git a/lychee-lib/src/types/response.rs b/lychee-lib/src/types/response.rs index 24085c7a88..d46ad1e27a 100644 --- a/lychee-lib/src/types/response.rs +++ b/lychee-lib/src/types/response.rs @@ -79,19 +79,24 @@ pub struct ResponseBody { // matching in these cases. impl Display for ResponseBody { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Always write the URI write!(f, "{}", self.uri)?; - if let Status::Ok(StatusCode::OK) = self.status { - // Don't print anything else if the status code is 200. - // The output gets too verbose then. + // Early return for OK status to avoid verbose output + if matches!(self.status, Status::Ok(StatusCode::OK)) { return Ok(()); } - // Add a separator between the URI and the additional details below. - // Note: To make the links clickable in some terminals, - // we add a space before the separator. - write!(f, " | {}", self.status)?; + // Format status and return early if empty + let status_output = self.status.to_string(); + if status_output.is_empty() { + return Ok(()); + } + + // Write status with separator + write!(f, " | {}", status_output)?; + // Add details if available if let Some(details) = self.status.details() { write!(f, ": {details}") } else { diff --git a/lychee-lib/src/types/status.rs b/lychee-lib/src/types/status.rs index 39ef79b0b2..bb845c715e 100644 --- a/lychee-lib/src/types/status.rs +++ b/lychee-lib/src/types/status.rs @@ -48,12 +48,12 @@ impl Display for Status { Status::Ok(code) => write!(f, "{code}"), Status::Redirected(code) => write!(f, "Redirect ({code})"), Status::UnknownStatusCode(code) => write!(f, "Unknown status ({code})"), - Status::Excluded => f.write_str("Excluded"), Status::Timeout(Some(code)) => write!(f, "Timeout ({code})"), Status::Timeout(None) => f.write_str("Timeout"), Status::Unsupported(e) => write!(f, "Unsupported: {e}"), Status::Error(e) => write!(f, "{e}"), Status::Cached(status) => write!(f, "{status}"), + Status::Excluded => Ok(()), } } }