diff --git a/.github/workflows/run-dev-tests.yml b/.github/workflows/run-dev-tests.yml index 8d1208b2..39cf8385 100644 --- a/.github/workflows/run-dev-tests.yml +++ b/.github/workflows/run-dev-tests.yml @@ -163,9 +163,8 @@ jobs: - name: Generate Coverage lcov report run: nur test lcov - - uses: codecov/codecov-action@e79a6962e0d4c0c17b229090214935d2e33f8354 # v6.0.1 + - uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: token: ${{secrets.CODECOV_TOKEN}} files: lcov.info fail_ci_if_error: true # optional (default = false) - use_pypi: true # workaround unexpected GPG key changes (temporarily) diff --git a/Cargo.lock b/Cargo.lock index d4a8c68b..59254ca6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -732,9 +732,9 @@ dependencies = [ [[package]] name = "git-bot-feedback" -version = "0.5.5" +version = "0.5.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2188a95438d5b5a329cd55f422c88cdc6efd8a191f88e1167270bb2ff8bcaee3" +checksum = "87c5dcb915266b670fbc0f47f7240cfe832a0e9363e7f509ef38dec960b80824" dependencies = [ "async-trait", "chrono", diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml index 6ea5939c..41c18957 100644 --- a/cpp-linter/Cargo.toml +++ b/cpp-linter/Cargo.toml @@ -33,7 +33,7 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } [dependencies.git-bot-feedback] -version = "0.5.5" +version = "0.5.6" features = ["file-changes"] # path = "../../git-bot-feedback" # git = "https://github.com/2bndy5/git-bot-feedback" diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index 0ef78100..35d51fdf 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -13,12 +13,12 @@ use gix_imara_diff::{Diff, InternedInput}; use log::Level; // project-specific crates/modules -use super::MakeSuggestions; +use super::{CACHE_DIR, MakeSuggestions}; use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct FormatAdvice { - /// A list of [`Replacement`]s that clang-tidy wants to make. + /// A list of line ranges that clang-format wants to replace. pub replacements: Vec>, pub patched: PathBuf, @@ -73,13 +73,14 @@ pub fn run_clang_format( .as_ref() .ok_or(ClangCaptureError::ToolPathUnknown("clang-format"))?; let mut cmd = Command::new(cmd_path); + cmd.current_dir(&clang_params.repo_root); let mut logs = vec![]; cmd.args(["--style", &clang_params.style]); let ranges = file.get_ranges(&clang_params.lines_changed_only); for range in &ranges { cmd.arg(format!("--lines={}:{}", range.start(), range.end())); } - let cache_path = clang_params.project_cache_dir.join("patches"); + let cache_path = clang_params.repo_root.join(CACHE_DIR).join("patches"); let cache_format_fixes = cache_path.join(file.name.with_added_extension("format")); fs::create_dir_all( cache_format_fixes @@ -125,9 +126,11 @@ pub fn run_clang_format( // use a diff between patched and original contents to get format results let original_contents = - fs::read_to_string(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed { - file_name: file_name.clone(), - source: e, + fs::read_to_string(clang_params.repo_root.join(&file.name)).map_err(|e| { + ClangCaptureError::ReadFileFailed { + file_name: file_name.clone(), + source: e, + } })?; let patched_contents = String::from_utf8(output.stdout.to_vec()).map_err(|e| { ClangCaptureError::NonUtf8Output { @@ -143,7 +146,7 @@ pub fn run_clang_format( .hunks() .filter_map(|hunk| { let replacement = if hunk.is_pure_insertion() { - RangeInclusive::new(hunk.after.start, hunk.after.end.saturating_sub(1)) + RangeInclusive::new(hunk.after.start, hunk.after.start) } else { RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1)) }; diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 111767e0..bc7201b4 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -2,9 +2,9 @@ //! output. use std::{ - env::{consts::OS, current_dir}, + env::consts::OS, fs, - path::PathBuf, + path::{Path, PathBuf}, process::Command, sync::{Arc, Mutex, MutexGuard}, }; @@ -144,13 +144,13 @@ const NOTE_HEADER: &str = r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+),?[ fn parse_tidy_output( tidy_stdout: &[u8], database_json: &Option>, + repo_root: &Path, ) -> Result { let note_header = Regex::new(NOTE_HEADER)?; let fixed_note = Regex::new(r"^.+:(\d+):\d+:\snote: FIX-IT applied suggested code changes$")?; let mut found_fix = false; let mut notification = None; let mut result = Vec::new(); - let cur_dir = current_dir().map_err(ClangCaptureError::UnknownWorkingDirectory)?; for line in String::from_utf8(tidy_stdout.to_vec()) .map_err(|e| ClangCaptureError::NonUtf8Output { task: "convert clang-tidy stdout".to_string(), @@ -191,16 +191,16 @@ fn parse_tidy_output( // file was not a named unit in the database; // try to normalize path as if relative to working directory. // NOTE: This shouldn't happen with a properly formed JSON database - filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename])); + filename = normalize_path(&PathBuf::from_iter([repo_root, &filename])); } } else { // still need to normalize the relative path despite missing database info. // let's assume the file is relative to current working directory. - filename = normalize_path(&PathBuf::from_iter([&cur_dir, &filename])); + filename = normalize_path(&PathBuf::from_iter([repo_root, &filename])); } - debug_assert!(filename.is_absolute()); + if filename.is_absolute() - && let Ok(file_n) = filename.strip_prefix(&cur_dir) + && let Ok(file_n) = filename.strip_prefix(repo_root) { // if this filename can't be made into a relative path, then it is // likely not a member of the project's sources (ie /usr/include/stdio.h) @@ -273,6 +273,7 @@ pub fn run_clang_tidy( .as_ref() .ok_or(ClangCaptureError::ToolPathUnknown("clang-tidy"))?; let mut cmd = Command::new(cmd_path); + cmd.current_dir(&clang_params.repo_root); let mut logs = vec![]; if !clang_params.tidy_checks.is_empty() { cmd.args(["-checks", &clang_params.tidy_checks]); @@ -296,16 +297,17 @@ pub fn run_clang_tidy( ); cmd.args(["--line-filter", filter.as_str()]); } + let repo_file_path = clang_params.repo_root.join(&file.name); let original_content = if !clang_params.tidy_review { None } else { cmd.arg("--fix-errors"); - Some( - fs::read_to_string(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed { + Some(fs::read_to_string(&repo_file_path).map_err(|e| { + ClangCaptureError::ReadFileFailed { file_name: file_name.clone(), source: e, - })?, - ) + } + })?) }; if !clang_params.style.is_empty() { cmd.args(["--format-style", clang_params.style.as_str()]); @@ -347,6 +349,7 @@ pub fn run_clang_tidy( file.tidy_advice = Some(parse_tidy_output( &output.stdout, &clang_params.database_json, + &clang_params.repo_root, )?); if clang_params.tidy_review && let Some(original_content) = &original_content @@ -355,14 +358,14 @@ pub fn run_clang_tidy( // cache file changes in a buffer and restore the original contents for further analysis tidy_advice.patched = Some( - fs::read(&file_name).map_err(|e| ClangCaptureError::ReadFileFailed { + fs::read(&repo_file_path).map_err(|e| ClangCaptureError::ReadFileFailed { file_name: file_name.clone(), source: e, })?, ); } // original_content is guaranteed to be Some() value at this point - fs::write(&file_name, original_content).map_err(|e| { + fs::write(&repo_file_path, original_content).map_err(|e| { ClangCaptureError::WriteFileFailed { file_name, source: e, @@ -496,7 +499,7 @@ mod test { format_review: false, clang_tidy_command: Some(exe_path), clang_format_command: None, - project_cache_dir: PathBuf::from(".").join(".cpp-linter-cache"), + repo_root: PathBuf::from("."), }; let mut file_lock = arc_file.lock().unwrap(); let logs = run_clang_tidy(&mut file_lock, &clang_params) @@ -564,7 +567,7 @@ TrenchBroom/TrenchBroom/common/test/src/mdl/tst_ReadFreeImageTexture.cpp:44:48: 44 | return diskFS.openFile(name) | kdl::and_then([](const auto& file) { | ^ "#; - let advice = parse_tidy_output(tidy_out.as_bytes(), &None).unwrap(); + let advice = parse_tidy_output(tidy_out.as_bytes(), &None, &PathBuf::from(".")).unwrap(); assert_eq!(advice.notes.len(), 4); for note in advice.notes { assert!(note.diagnostic.contains('-')); diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 62127d1b..e5312ccb 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -17,15 +17,18 @@ use tokio::task::JoinSet; // project-specific modules/crates use super::common_fs::FileObj; -use crate::error::{ClangCaptureError, ClangTaskError}; use crate::{ + clang_tools::clang_tidy::CompilationUnit, cli::ClangParams, + error::{ClangCaptureError, ClangTaskError}, rest_client::{RestClient, USER_OUTREACH}, }; pub mod clang_format; use clang_format::run_clang_format; pub mod clang_tidy; -use clang_tidy::{CompilationUnit, run_clang_tidy}; +use clang_tidy::run_clang_tidy; + +pub const CACHE_DIR: &str = ".cpp-linter-cache"; /// This creates a task to run clang-tidy and clang-format on a single file. /// @@ -135,15 +138,27 @@ pub async fn capture_clang_tools_output( clang_versions.format_version = Some(tool_info.version); clang_params.clang_format_command = Some(tool_info.path); } - - // parse database (if provided) to match filenames when parsing clang-tidy's stdout - if let Some(db_path) = &clang_params.database - && let Ok(db_str) = fs::read(db_path.join("compile_commands.json")) - { - clang_params.database_json = Some( - // A compilation database should be UTF-8 encoded, but file paths are not; use lossy conversion. - serde_json::from_str::>(&String::from_utf8_lossy(&db_str))?, - ) + if let Some(db_path) = &clang_params.database { + let db_path = db_path.join("compile_commands.json"); + match fs::read_to_string(&db_path) { + Ok(db_str) => match serde_json::from_str::>(&db_str) { + Ok(db_json) => { + clang_params.database_json = Some(db_json); + } + Err(e) => { + log::warn!( + "Failed to parse compilation database JSON at {}: {e:?}", + db_path.to_string_lossy() + ); + } + }, + Err(e) => { + log::warn!( + "Failed to read compilation database file at {}: {e:?}", + db_path.to_string_lossy() + ); + } + } }; let mut executors = JoinSet::new(); @@ -382,3 +397,44 @@ pub trait MakeSuggestions { *tool_total += hunks_in_patch; } } + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used)] + + use std::{env, fs, path::Path}; + + use super::*; + #[cfg(feature = "bin")] + use crate::logger::try_init; + + async fn test_db_parse>(path: P) -> Result { + let clang_params = ClangParams { + database: Some(path.as_ref().to_path_buf()), + repo_root: PathBuf::from("."), + ..Default::default() + }; + let version = RequestedVersion::default(); + // We don't need to use any specific git REST API client for this. + unsafe { + env::remove_var("GITHUB_ACTIONS"); + } + let rest_client = RestClient::new().unwrap(); + #[cfg(feature = "bin")] + try_init(); + capture_clang_tools_output(&[], &version, clang_params, &rest_client).await + } + + #[tokio::test] + async fn bad_db_path() { + test_db_parse("nonexistent/path").await.unwrap(); + } + + #[tokio::test] + async fn bad_db_json() { + let tmp_dir = tempfile::tempdir().unwrap(); + let db_path = tmp_dir.path().join("compile_commands.json"); + fs::write(&db_path, "not a valid json").unwrap(); + test_db_parse(tmp_dir.path()).await.unwrap(); + } +} diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index e66cf796..7a4a68bc 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -151,16 +151,15 @@ pub struct SourceOptions { )] pub extensions: Vec, - /// The relative path to the repository root directory. + /// The path to the repository root directory. /// - /// This path is relative to the runner's `GITHUB_WORKSPACE` - /// environment variable (or the current working directory if - /// not using a CI runner). + /// This path should be relative to the current + /// working directory. It can also be an absolute path. #[cfg_attr( feature = "bin", arg(short, long, default_value = ".", help_heading = "Source options") )] - pub repo_root: String, + pub repo_root: PathBuf, /// This controls what part of the files are analyzed. #[cfg_attr( diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index 48e473f9..572234be 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -178,7 +178,7 @@ pub struct ClangParams { pub format_filter: Option, pub tidy_review: bool, pub format_review: bool, - pub project_cache_dir: PathBuf, + pub repo_root: PathBuf, } #[cfg(feature = "bin")] @@ -203,10 +203,23 @@ impl From<&Cli> for ClangParams { let ignore_format: Vec<&str> = ignore_format.iter().map(|s| s.as_str()).collect(); FileFilter::new(&ignore_format, &extensions, Some("clang-format")) }); + let repo_root = args.source_options.repo_root.clone(); + let database = args + .tidy_options + .database + .as_ref() + .map(PathBuf::from) + .map(|db| { + if db.is_relative() { + repo_root.join(db) + } else { + db + } + }); ClangParams { tidy_checks: args.tidy_options.tidy_checks.clone(), lines_changed_only: args.source_options.lines_changed_only.clone(), - database: args.tidy_options.database.clone(), + database, extra_args: args.tidy_options.extra_arg.clone(), database_json: None, style: args.format_options.style.clone(), @@ -216,8 +229,7 @@ impl From<&Cli> for ClangParams { format_filter, tidy_review: args.feedback_options.tidy_review, format_review: args.feedback_options.format_review, - project_cache_dir: PathBuf::from(&args.source_options.repo_root) - .join(".cpp-linter-cache"), + repo_root, } } } @@ -233,6 +245,7 @@ pub struct FeedbackInput { pub tidy_review: bool, pub format_review: bool, pub passive_reviews: bool, + pub repo_root: PathBuf, } #[cfg(feature = "bin")] @@ -248,6 +261,7 @@ impl From<&Cli> for FeedbackInput { tidy_review: args.feedback_options.tidy_review, format_review: args.feedback_options.format_review, passive_reviews: args.feedback_options.passive_reviews, + repo_root: args.source_options.repo_root.clone(), } } } @@ -264,6 +278,7 @@ impl Default for FeedbackInput { tidy_review: false, format_review: false, passive_reviews: false, + repo_root: PathBuf::from("."), } } } @@ -274,7 +289,7 @@ mod test { use clap::{Parser, ValueEnum}; - use super::{Cli, LinesChangedOnly, ThreadComments}; + use super::{ClangParams, Cli, LinesChangedOnly, ThreadComments}; #[test] fn parse_positional() { @@ -313,4 +328,12 @@ mod test { ThreadComments::Off ); } + + #[test] + fn absolute_db_path() { + let tmp_dir = tempfile::tempdir().unwrap(); + let cli = Cli::parse_from(["cpp-linter", "--database", tmp_dir.path().to_str().unwrap()]); + let clang_params = ClangParams::from(&cli); + assert_eq!(clang_params.database, Some(tmp_dir.path().to_path_buf())); + } } diff --git a/cpp-linter/src/common_fs/mod.rs b/cpp-linter/src/common_fs/mod.rs index 51ab07fb..cd78b340 100644 --- a/cpp-linter/src/common_fs/mod.rs +++ b/cpp-linter/src/common_fs/mod.rs @@ -1,6 +1,11 @@ //! A module to hold all common file system functionality. -use std::{fmt::Debug, fs, ops::RangeInclusive, path::PathBuf}; +use std::{ + fmt::Debug, + fs, + ops::RangeInclusive, + path::{Path, PathBuf}, +}; use gix_imara_diff::Hunk; @@ -131,7 +136,7 @@ impl FileObj { } /// Create a list of [`Suggestion`](struct@crate::clang_tools::Suggestion) from a - /// generated [`Patch`](struct@git2::Patch) and store them in the given + /// generated diff and store them in the given /// [`ReviewComments`](struct@crate::clang_tools::ReviewComments). /// /// The suggestions will also include diagnostics from clang-tidy that @@ -140,8 +145,10 @@ impl FileObj { &self, review_comments: &mut ReviewComments, summary_only: bool, + repo_root: &Path, ) -> Result<(), FileObjError> { - let original_content = fs::read_to_string(&self.name).map_err(FileObjError::ReadFile)?; + let original_content = + fs::read_to_string(repo_root.join(&self.name)).map_err(FileObjError::ReadFile)?; let file_name = self.name.to_str().unwrap_or_default().replace("\\", "/"); if let Some(advice) = &self.format_advice { let patched = fs::read_to_string(&advice.patched).map_err(FileObjError::ReadFile)?; diff --git a/cpp-linter/src/git.rs b/cpp-linter/src/git.rs index a77cf7d0..03c7cb44 100644 --- a/cpp-linter/src/git.rs +++ b/cpp-linter/src/git.rs @@ -9,6 +9,7 @@ mod test { use std::{ env::{self, current_dir, set_current_dir}, fs, + path::PathBuf, process::Command, }; @@ -101,6 +102,7 @@ mod test { &LinesChangedOnly::Off.into(), &base_diff, ignore_staged, + &PathBuf::from("."), ) .await .unwrap() diff --git a/cpp-linter/src/rest_client/mod.rs b/cpp-linter/src/rest_client/mod.rs index cec9af4e..e330af87 100644 --- a/cpp-linter/src/rest_client/mod.rs +++ b/cpp-linter/src/rest_client/mod.rs @@ -1,6 +1,6 @@ use std::{ env, - path::PathBuf, + path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -54,6 +54,7 @@ impl RestClient { lines_changed_only: &LinesChangedOnly, base_diff: &Option, ignore_index: bool, + repo_root: &Path, ) -> Result, ClientError> { let files = self .client @@ -72,8 +73,12 @@ impl RestClient { .iter() .map(|hunk| hunk.start..=hunk.end) .collect(); + let file_path = PathBuf::from(file_name); FileObj::from( - PathBuf::from(&file_name), + file_path + .strip_prefix(repo_root) + .map(PathBuf::from) + .unwrap_or(file_path), diff_lines.added_lines.clone(), diff_chunks, ) @@ -176,7 +181,11 @@ impl RestClient { let file = file .lock() .map_err(|e| ClientError::MutexPoisoned(e.to_string()))?; - file.make_suggestions_from_patch(&mut review_comments, summary_only)?; + file.make_suggestions_from_patch( + &mut review_comments, + summary_only, + &feedback_inputs.repo_root, + )?; } let mut options = ReviewOptions { diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 9746a74f..2aa5e6d0 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -6,7 +6,7 @@ #![cfg(feature = "bin")] use std::{ env, - path::{Path, PathBuf}, + path::PathBuf, sync::{Arc, Mutex}, }; @@ -18,7 +18,7 @@ use log::{LevelFilter, set_max_level}; // project specific modules/crates use crate::{ - clang_tools::capture_clang_tools_output, + clang_tools::{CACHE_DIR, capture_clang_tools_output}, cli::{ClangParams, Cli, CliCommand, FeedbackInput, LinesChangedOnly}, common_fs::FileObj, logger, @@ -59,15 +59,6 @@ pub async fn run_main(args: Vec) -> Result<()> { logger::try_init(); - if cli.source_options.repo_root != "." { - env::set_current_dir(Path::new(&cli.source_options.repo_root)).map_err(|e| { - anyhow!( - "'{}' is inaccessible or does not exist: {e:?}", - cli.source_options.repo_root - ) - })?; - } - let mut rest_api_client = RestClient::new()?; set_max_level( if cli.general_options.verbosity.is_debug() || rest_api_client.is_debug_enabled() { @@ -110,6 +101,7 @@ pub async fn run_main(args: Vec) -> Result<()> { } rest_api_client.start_log_group("Get list of specified source files"); + let repo_root_path = PathBuf::from(&cli.source_options.repo_root); let files = if !matches!(cli.source_options.lines_changed_only, LinesChangedOnly::Off) || cli.source_options.files_changed_only { @@ -120,14 +112,23 @@ pub async fn run_main(args: Vec) -> Result<()> { &cli.source_options.lines_changed_only.clone().into(), &cli.source_options.diff_base, cli.source_options.ignore_index, + &repo_root_path, ) .await? } else { // walk the folder and look for files with specified extensions according to ignore values. let mut all_files: Vec = file_filter - .walk_dir(".")? + .walk_dir(&cli.source_options.repo_root)? .into_iter() - .map(|file_name| FileObj::new(PathBuf::from(&file_name))) + .map(|file_name| { + let file_path = PathBuf::from(&file_name); + FileObj::new( + file_path + .strip_prefix(&repo_root_path) + .map(PathBuf::from) + .unwrap_or(file_path), + ) + }) .collect(); if is_pr && (cli.feedback_options.tidy_review || cli.feedback_options.format_review) { let changed_files = rest_api_client @@ -136,6 +137,7 @@ pub async fn run_main(args: Vec) -> Result<()> { &LinesChangedOnly::Off.into(), &cli.source_options.diff_base, cli.source_options.ignore_index, + &repo_root_path, ) .await?; for changed_file in changed_files { @@ -160,11 +162,12 @@ pub async fn run_main(args: Vec) -> Result<()> { let mut clang_params = ClangParams::from(&cli); // mkdir -p .cpp-linter-cache/ - std::fs::create_dir_all(&clang_params.project_cache_dir) + let cache_dir = clang_params.repo_root.join(CACHE_DIR); + std::fs::create_dir_all(&cache_dir) .with_context(|| "Failed to create a local cache directory.")?; // add gitignore file in project cache dir std::fs::write( - clang_params.project_cache_dir.join(".gitignore"), + cache_dir.join(".gitignore"), "# Automatically created by cpp-linter\n*\n", ) .with_context(|| "Failed to write .cpp-linter-cache/.gitignore file")?; @@ -296,17 +299,4 @@ mod test { .unwrap(); drop(tmp_gh_out); } - - #[tokio::test] - async fn bad_repo_root() { - let tmp_gh_out = setup_tmp_gh_out_path(); - run_main(vec![ - "cpp-linter".to_string(), - "--repo-root".to_string(), - "some-non-existent-dir".to_string(), - ]) - .await - .unwrap_err(); - drop(tmp_gh_out); - } } diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index b77aa13d..45656745 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -5,7 +5,7 @@ use cpp_linter::run::run_main; use git_bot_feedback::LinesChangedOnly; use mockito::Matcher; use std::{env, fmt::Display, io::Write, path::Path}; -use tempfile::NamedTempFile; +use tempfile::{NamedTempFile, TempDir}; mod common; use common::{create_test_space, mock_server}; @@ -62,7 +62,7 @@ impl Default for TestParams { } } -async fn setup(lib_root: &Path, test_params: &TestParams) { +async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { let mut event_payload_path = NamedTempFile::new_in("./").unwrap(); let tmp_out_file = NamedTempFile::new().unwrap(); unsafe { @@ -253,6 +253,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { format!("--no-lgtm={}", test_params.no_lgtm), "-p=build".to_string(), "-i=build".to_string(), + format!("--repo-root={}", tmp_dir.path().to_str().unwrap()), ]; if test_params.force_lgtm { args.push("-e=c".to_string()); @@ -278,9 +279,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { async fn test_comment(test_params: &TestParams) { let tmp_dir = create_test_space(true); let lib_root = env::current_dir().unwrap(); - env::set_current_dir(tmp_dir.path()).unwrap(); - setup(&lib_root, test_params).await; - env::set_current_dir(lib_root.as_path()).unwrap(); + setup(&lib_root, &tmp_dir, test_params).await; drop(tmp_dir); } diff --git a/cpp-linter/tests/common/mod.rs b/cpp-linter/tests/common/mod.rs index b73e99d6..10a72618 100644 --- a/cpp-linter/tests/common/mod.rs +++ b/cpp-linter/tests/common/mod.rs @@ -21,7 +21,7 @@ use tempfile::TempDir; /// The returned directory object will automatically delete the /// temporary folder when it is dropped out of scope. pub fn create_test_space(setup_meson: bool) -> TempDir { - let tmp = TempDir::new().unwrap(); + let tmp = TempDir::with_prefix("cpp-linter-test").unwrap(); fs::create_dir(tmp.path().join("src")).unwrap(); let src = fs::read_dir("tests/demo").unwrap(); for file in src { @@ -48,11 +48,10 @@ pub fn create_test_space(setup_meson: bool) -> TempDir { "demo.cpp", "demo.hpp", ]); - let output = cmd.output().expect("Failed to run 'meson init'"); - println!( - "meson init stdout:\n{}", - String::from_utf8(output.stdout.to_vec()).unwrap() - ); + let status = cmd.status().expect("Failed to run 'meson init'"); + if !status.success() { + panic!("Failed to initialize meson project with 'meson init'"); + } let meson_build_dir = tmp.path().join("build"); let mut cmd = Command::new("meson"); cmd.args([ @@ -61,13 +60,12 @@ pub fn create_test_space(setup_meson: bool) -> TempDir { meson_build_dir.as_path().to_str().unwrap(), test_dir.to_str().unwrap(), ]); - let output = cmd - .output() + let status = cmd + .status() .expect("Failed to generate build assets with 'meson setup'"); - println!( - "meson setup stdout:\n{}", - String::from_utf8(output.stdout.to_vec()).unwrap() - ); + if !status.success() { + panic!("Failed to generate build assets with 'meson setup'"); + } let db = fs::File::open(meson_build_dir.join("compile_commands.json")) .expect("Failed to open compilation database"); for line in io::BufReader::new(db).lines().map_while(Result::ok) { diff --git a/cpp-linter/tests/paginated_changed_files.rs b/cpp-linter/tests/paginated_changed_files.rs index f8b57001..7563a10b 100644 --- a/cpp-linter/tests/paginated_changed_files.rs +++ b/cpp-linter/tests/paginated_changed_files.rs @@ -32,9 +32,8 @@ const RESET_RATE_LIMIT_HEADER: &str = "x-ratelimit-reset"; const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining"; const MALFORMED_RESPONSE_PAYLOAD: &str = "{\"message\":\"Resource not accessible by integration\"}"; -async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { - let tmp = TempDir::new().expect("Failed to create a temp dir for test"); - let mut event_payload = NamedTempFile::new_in(tmp.path()) +async fn get_paginated_changes(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { + let mut event_payload = NamedTempFile::new_in(tmp_dir.path()) .expect("Failed to spawn a tmp file for test event payload"); unsafe { env::set_var("GITHUB_ACTIONS", "true"); @@ -84,7 +83,6 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { unsafe { env::set_var("GITHUB_API_URL", server.url()); } - env::set_current_dir(tmp.path()).unwrap(); logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let gh_client = RestClient::new(); @@ -140,7 +138,13 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { let file_filter = FileFilter::new(&[], &["cpp", "hpp"], None); let files = client - .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off, &None::, false) + .get_list_of_changed_files( + &file_filter, + &LinesChangedOnly::Off, + &None::, + false, + tmp_dir.path(), + ) .await; match files { Err(e) => { @@ -171,9 +175,7 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { async fn test_get_changes(test_params: &TestParams) { let tmp_dir = create_test_space(false); let lib_root = env::current_dir().unwrap(); - env::set_current_dir(tmp_dir.path()).unwrap(); - get_paginated_changes(&lib_root, test_params).await; - env::set_current_dir(lib_root.as_path()).unwrap(); + get_paginated_changes(&lib_root, &tmp_dir, test_params).await; drop(tmp_dir); } diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index b13c16ac..3d6134d7 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -7,7 +7,7 @@ use cpp_linter::{ use git_bot_feedback::LinesChangedOnly; use mockito::Matcher; use std::{env, io::Write, path::Path}; -use tempfile::NamedTempFile; +use tempfile::{NamedTempFile, TempDir}; mod common; use common::{create_test_space, mock_server}; @@ -70,7 +70,7 @@ fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str } } -async fn setup(lib_root: &Path, test_params: &TestParams) { +async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { let mut event_payload_path = NamedTempFile::new_in("./").unwrap(); let event_payload = if test_params.bad_pr_info { "".to_string() @@ -239,6 +239,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { format!("--no-lgtm={}", test_params.no_lgtm), "-p=build".to_string(), "-i=build".to_string(), + format!("--repo-root={}", tmp_dir.path().to_str().unwrap()), ]; if test_params.force_lgtm { if test_params.tidy_review { @@ -272,9 +273,7 @@ async fn setup(lib_root: &Path, test_params: &TestParams) { async fn test_review(test_params: &TestParams) { let tmp_dir = create_test_space(true); let lib_root = env::current_dir().unwrap(); - env::set_current_dir(tmp_dir.path()).unwrap(); - setup(&lib_root, test_params).await; - env::set_current_dir(lib_root.as_path()).unwrap(); + setup(&lib_root, &tmp_dir, test_params).await; drop(tmp_dir); }