From a0b0ace061e797e26dc764b5a3e41f7440864a8c Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 25 Oct 2023 10:51:29 +1100 Subject: [PATCH] tidy: add unit tests for alphabetical markers. This requires introducing `tidy_error_ext!` as an alternative to `tidy_error!`. It is passed a closure that is called for an error. This lets tests capture tidy error messages in a buffer instead of printing them to stderr. It also requires pulling part of `check` out into a new function `check_lines`, so that tests can pass in an iterator over some strings instead of a file. --- src/tools/tidy/src/alphabetical.rs | 59 ++++--- src/tools/tidy/src/alphabetical/tests.rs | 188 +++++++++++++++++++++++ src/tools/tidy/src/lib.rs | 16 +- 3 files changed, 238 insertions(+), 25 deletions(-) create mode 100644 src/tools/tidy/src/alphabetical/tests.rs diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 32fb16b0a4214..150a9594350ed 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -24,6 +24,9 @@ use std::path::Path; use crate::walk::{filter_dirs, walk}; +#[cfg(test)] +mod tests; + fn indentation(line: &str) -> usize { line.find(|c| c != ' ').unwrap_or(0) } @@ -38,6 +41,7 @@ const END_MARKER: &str = "tidy-alphabetical-end"; fn check_section<'a>( file: impl Display, lines: impl Iterator, + err: &mut dyn FnMut(&str) -> std::io::Result<()>, bad: &mut bool, ) { let mut prev_line = String::new(); @@ -50,7 +54,12 @@ fn check_section<'a>( } if line.contains(START_MARKER) { - tidy_error!(bad, "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", idx + 1); + tidy_error_ext!( + err, + bad, + "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`", + idx + 1 + ); return; } @@ -93,32 +102,44 @@ fn check_section<'a>( let prev_line_trimmed_lowercase = prev_line.trim_start_matches(' ').to_lowercase(); if trimmed_line.to_lowercase() < prev_line_trimmed_lowercase { - tidy_error!(bad, "{file}:{}: line not in alphabetical order", idx + 1); + tidy_error_ext!(err, bad, "{file}:{}: line not in alphabetical order", idx + 1); } prev_line = line; } - tidy_error!(bad, "{file}: reached end of file expecting `{END_MARKER}`") + tidy_error_ext!(err, bad, "{file}: reached end of file expecting `{END_MARKER}`") } -pub fn check(path: &Path, bad: &mut bool) { - walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| { - let file = &entry.path().display(); +fn check_lines<'a>( + file: &impl Display, + mut lines: impl Iterator, + err: &mut dyn FnMut(&str) -> std::io::Result<()>, + bad: &mut bool, +) { + while let Some((idx, line)) = lines.next() { + if line.contains(END_MARKER) { + tidy_error_ext!( + err, + bad, + "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`", + idx + 1 + ) + } - let mut lines = contents.lines().enumerate(); - while let Some((idx, line)) = lines.next() { - if line.contains(END_MARKER) { - tidy_error!( - bad, - "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`", - idx + 1 - ) - } - - if line.contains(START_MARKER) { - check_section(file, &mut lines, bad); - } + if line.contains(START_MARKER) { + check_section(file, &mut lines, err, bad); } + } +} + +pub fn check(path: &Path, bad: &mut bool) { + let skip = + |path: &_, _is_dir| filter_dirs(path) || path.ends_with("tidy/src/alphabetical/tests.rs"); + + walk(path, skip, &mut |entry, contents| { + let file = &entry.path().display(); + let lines = contents.lines().enumerate(); + check_lines(file, lines, &mut crate::tidy_error, bad) }); } diff --git a/src/tools/tidy/src/alphabetical/tests.rs b/src/tools/tidy/src/alphabetical/tests.rs new file mode 100644 index 0000000000000..560e0284b89a1 --- /dev/null +++ b/src/tools/tidy/src/alphabetical/tests.rs @@ -0,0 +1,188 @@ +use super::*; +use std::io::Write; +use std::str::from_utf8; + +fn test(lines: &str, name: &str, expected_msg: &str, expected_bad: bool) { + let mut actual_msg = Vec::new(); + let mut actual_bad = false; + let mut err = |args: &_| { + write!(&mut actual_msg, "{args}")?; + Ok(()) + }; + check_lines(&name, lines.lines().enumerate(), &mut err, &mut actual_bad); + assert_eq!(expected_msg, from_utf8(&actual_msg).unwrap()); + assert_eq!(expected_bad, actual_bad); +} + +fn good(lines: &str) { + test(lines, "good", "", false); +} + +fn bad(lines: &str, expected_msg: &str) { + test(lines, "bad", expected_msg, true); +} + +#[test] +fn test_no_markers() { + let lines = "\ + def + abc + xyz + "; + good(lines); +} + +#[test] +fn test_rust_good() { + let lines = "\ + // tidy-alphabetical-start + abc + def + xyz + // tidy-alphabetical-end"; // important: end marker on last line + good(lines); +} + +#[test] +fn test_complex_good() { + let lines = "\ + zzz + + // tidy-alphabetical-start + abc + // Rust comments are ok + def + # TOML comments are ok + xyz + // tidy-alphabetical-end + + # tidy-alphabetical-start + foo(abc); + // blank lines are ok + + // split line gets joined + foo( + def + ); + + foo(xyz); + # tidy-alphabetical-end + + % tidy-alphabetical-start + abc + ignored_due_to_different_indent + def + % tidy-alphabetical-end + + aaa + "; + good(lines); +} + +#[test] +fn test_rust_bad() { + let lines = "\ + // tidy-alphabetical-start + abc + xyz + def + // tidy-alphabetical-end + "; + bad(lines, "bad:4: line not in alphabetical order"); +} + +#[test] +fn test_toml_bad() { + let lines = "\ + # tidy-alphabetical-start + abc + xyz + def + # tidy-alphabetical-end + "; + bad(lines, "bad:4: line not in alphabetical order"); +} + +#[test] +fn test_features_bad() { + // Even though lines starting with `#` are treated as comments, lines + // starting with `#!` are an exception. + let lines = "\ + tidy-alphabetical-start + #![feature(abc)] + #![feature(xyz)] + #![feature(def)] + tidy-alphabetical-end + "; + bad(lines, "bad:4: line not in alphabetical order"); +} + +#[test] +fn test_indent_bad() { + // All lines are indented the same amount, and so are checked. + let lines = "\ + $ tidy-alphabetical-start + abc + xyz + def + $ tidy-alphabetical-end + "; + bad(lines, "bad:4: line not in alphabetical order"); +} + +#[test] +fn test_split_bad() { + let lines = "\ + || tidy-alphabetical-start + foo(abc) + foo( + xyz + ) + foo( + def + ) + && tidy-alphabetical-end + "; + bad(lines, "bad:7: line not in alphabetical order"); +} + +#[test] +fn test_double_start() { + let lines = "\ + tidy-alphabetical-start + abc + tidy-alphabetical-start + "; + bad(lines, "bad:3 found `tidy-alphabetical-start` expecting `tidy-alphabetical-end`"); +} + +#[test] +fn test_missing_start() { + let lines = "\ + abc + tidy-alphabetical-end + abc + "; + bad(lines, "bad:2 found `tidy-alphabetical-end` expecting `tidy-alphabetical-start`"); +} + +#[test] +fn test_missing_end() { + let lines = "\ + tidy-alphabetical-start + abc + "; + bad(lines, "bad: reached end of file expecting `tidy-alphabetical-end`"); +} + +#[test] +fn test_double_end() { + let lines = "\ + tidy-alphabetical-start + abc + tidy-alphabetical-end + def + tidy-alphabetical-end + "; + bad(lines, "bad:5 found `tidy-alphabetical-end` expecting `tidy-alphabetical-start`"); +} diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index fc69c1432227e..eb0a2fda290d9 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,8 +3,6 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use std::fmt::Display; - use termcolor::WriteColor; /// A helper macro to `unwrap` a result except also print out details like: @@ -31,16 +29,22 @@ macro_rules! t { macro_rules! tidy_error { ($bad:expr, $($fmt:tt)*) => ({ - $crate::tidy_error($bad, format_args!($($fmt)*)).expect("failed to output error"); + $crate::tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error"); + *$bad = true; }); } -fn tidy_error(bad: &mut bool, args: impl Display) -> std::io::Result<()> { +macro_rules! tidy_error_ext { + ($tidy_error:path, $bad:expr, $($fmt:tt)*) => ({ + $tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error"); + *$bad = true; + }); +} + +fn tidy_error(args: &str) -> std::io::Result<()> { use std::io::Write; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream}; - *bad = true; - let mut stderr = StandardStream::stdout(ColorChoice::Auto); stderr.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?;