From 5fac1bba14ad5b9e4d2abfa3ca299f88cc1e62bf Mon Sep 17 00:00:00 2001 From: nsfisis Date: Mon, 20 Jul 2026 01:33:14 +0900 Subject: fix(process-executor): un-ignore 11 tests by implementing execute_async mock support ProcessExecutor::execute_async's mock branch was an unimplemented todo!(), blocking every test whose code path calls it (feature-branch git diffing, system-unzip/7z fallback extraction). Implement it by: - Adding Process::__mock (mirroring the existing ZipArchive::__mock pattern) so execute_async can resolve with a fabricated, already- terminated Process instead of spawning a real subprocess. - Extracting the sync mock's expectation-matching logic into a shared ProcessExecutor::mock_match, wrapping the mock state in a RefCell so it works from execute_async's &self/&mut self receivers. - Setting error_output/capture_output from the mock branch, matching PHP's ProcessExecutorMock::executeAsync sharing doExecute with the sync path; execute_async now takes &mut self for this (safe, since the borrow only needs to live through the synchronous setup, not across the .await). - Turning a strict-mode expectation mismatch from a panic!() into a shirabe_php_shim::RuntimeException Err, mirroring PHPUnit's AssertionFailedError extending \RuntimeException: PHP call sites that catch (\RuntimeException $e) around a mocked git/hg/svn call (e.g. Git::get_mirror_default_branch, GitDriver::supports) treat a mismatch as an ordinary recoverable failure, and now so does the port. The RefMut is dropped before firing an expectation's optional callback so a re-entrant callback doesn't panic on double-borrow. Also fixes two real bugs found while porting test_private_repository_ no_interaction: GitHub::authorize_oauth and GitLab::authorize_oauth checked their domains config via PhpMixed::as_array(), which only matches the Array (map) variant, but github-domains/gitlab-domains default to PhpMixed::List, so the check always returned false and OAuth token lookup was silently skipped. Use the in_array shim instead, matching PHP's in_array() semantics. Also fix Git::run_command's "capture credentials from git remote -v" call, which used the panic- swallowing execute_args wrapper instead of a fallible execute(), so a mock mismatch there couldn't reach get_mirror_default_branch's catch. Un-ignores: - zip_downloader_test::test_system_unzip_only_{good,failed} - zip_downloader_test::test_non_windows_fallback_{good,failed} - event_dispatcher_test::test_dispatcher_outputs_error_on_failed_command - root_package_loader_test::test_feature_branch_pretty_version - version_guesser_test::test_guess_version_reads_and_respects_non_feature_branches_configuration_for_arbitrary_naming{,_regex} - version_guesser_test::test_remote_branches_are_selected - github_driver_test::test_private_repository_no_interaction (also adds the missing #[serial], since it seeds the shared Git::VERSION static that vcs_repository_test::test_load_versions depends on for real) - init_command_test::test_get_git_config, made deterministic by pointing HOME at a throwaway dir with its own .gitconfig instead of depending on the host's global git config Deduplicates the GitVersionGuard/RestoreEnv test-drop-guard idioms into tests/common/test_case.rs instead of reimplementing them per file. Co-Authored-By: Claude Sonnet 5 --- crates/shirabe/src/downloader/zip_downloader.rs | 6 +- .../shirabe/src/package/version/version_guesser.rs | 5 +- crates/shirabe/src/util/filesystem.rs | 2 +- crates/shirabe/src/util/git.rs | 6 +- crates/shirabe/src/util/github.rs | 13 +- crates/shirabe/src/util/gitlab.rs | 20 +-- crates/shirabe/src/util/process_executor.rs | 167 ++++++++++++++------- 7 files changed, 144 insertions(+), 75 deletions(-) (limited to 'crates/shirabe/src') diff --git a/crates/shirabe/src/downloader/zip_downloader.rs b/crates/shirabe/src/downloader/zip_downloader.rs index 9fa68c53..f70846e4 100644 --- a/crates/shirabe/src/downloader/zip_downloader.rs +++ b/crates/shirabe/src/downloader/zip_downloader.rs @@ -130,7 +130,11 @@ impl ZipDownloader { // Build the future first so the executor borrow is released before awaiting; a borrow // held across the await would collide with sibling extracts or sync execute() calls. - let process_future = self.inner.process.borrow().execute_async(&command, None); + let process_future = self + .inner + .process + .borrow_mut() + .execute_async(&command, None); let process_result = process_future.await; match process_result { Ok(mut process) => { diff --git a/crates/shirabe/src/package/version/version_guesser.rs b/crates/shirabe/src/package/version/version_guesser.rs index 185616ac..2f98e339 100644 --- a/crates/shirabe/src/package/version/version_guesser.rs +++ b/crates/shirabe/src/package/version/version_guesser.rs @@ -576,7 +576,10 @@ impl VersionGuesser { }, &scm_cmdline, ); - let process_future = self.process.borrow().execute_async(&cmd_line, Some(path)); + let process_future = self + .process + .borrow_mut() + .execute_async(&cmd_line, Some(path)); let mut process = sync_executor::block_on(process_future)?; if !process.is_successful() { continue; diff --git a/crates/shirabe/src/util/filesystem.rs b/crates/shirabe/src/util/filesystem.rs index 70bf69f7..3516b284 100644 --- a/crates/shirabe/src/util/filesystem.rs +++ b/crates/shirabe/src/util/filesystem.rs @@ -200,7 +200,7 @@ impl Filesystem { (fs.get_process_handle(), cmd) }; - let process_future = process_executor.borrow().execute_async( + let process_future = process_executor.borrow_mut().execute_async( PhpMixed::List(cmd.iter().map(|s| PhpMixed::String(s.clone())).collect()), None, ); diff --git a/crates/shirabe/src/util/git.rs b/crates/shirabe/src/util/git.rs index 1e6768b1..6a6aaeb1 100644 --- a/crates/shirabe/src/util/git.rs +++ b/crates/shirabe/src/util/git.rs @@ -229,11 +229,11 @@ impl Git { if !initial_clone { // capture username/password from URL if there is one and we have no auth configured yet let mut output = String::new(); - self.process.borrow_mut().execute_args( - &["git".to_string(), "remote".to_string(), "-v".to_string()], + self.process.borrow_mut().execute( + &["git".to_string(), "remote".to_string(), "-v".to_string()][..], &mut output, cwd, - ); + )?; let mut m: IndexMap = IndexMap::new(); if Preg::is_match3( php_regex!(r"{^(?:composer|origin)\s+https?://(.+):(.+)@([^/]+)}im"), diff --git a/crates/shirabe/src/util/github.rs b/crates/shirabe/src/util/github.rs index e8e83ddd..e6bac7ec 100644 --- a/crates/shirabe/src/util/github.rs +++ b/crates/shirabe/src/util/github.rs @@ -9,7 +9,7 @@ use crate::util::HttpDownloader; use crate::util::ProcessExecutor; use indexmap::IndexMap; use shirabe_external_packages::composer::pcre::{CaptureKey, Preg}; -use shirabe_php_shim::{PhpMixed, date, php_regex, stripos, strtolower}; +use shirabe_php_shim::{PhpMixed, date, in_array, php_regex, stripos, strtolower}; #[derive(Debug)] pub struct GitHub { @@ -52,12 +52,11 @@ impl GitHub { pub fn authorize_oauth(&mut self, origin_url: &str) -> bool { let github_domains = self.config.borrow_mut().get("github-domains"); - let domains = match github_domains.as_array() { - Some(arr) => arr.clone(), - None => return false, - }; - let origin_in_domains = domains.values().any(|v| v.as_string() == Some(origin_url)); - if !origin_in_domains { + if !in_array( + PhpMixed::String(origin_url.to_string()), + &github_domains, + false, + ) { return false; } diff --git a/crates/shirabe/src/util/gitlab.rs b/crates/shirabe/src/util/gitlab.rs index 4a636efa..485d4126 100644 --- a/crates/shirabe/src/util/gitlab.rs +++ b/crates/shirabe/src/util/gitlab.rs @@ -11,7 +11,7 @@ use crate::util::ProcessExecutor; use indexmap::IndexMap; use shirabe_external_packages::composer::pcre::Preg; use shirabe_php_shim::{ - PhpMixed, RuntimeException, http_build_query, json_decode, php_regex, time, + PhpMixed, RuntimeException, http_build_query, in_array, json_decode, php_regex, time, }; #[derive(Debug)] @@ -55,15 +55,15 @@ impl GitLab { let bc_origin_url = Preg::replace(php_regex!("{:\\d+}"), "", origin_url); let gitlab_domains = self.config.borrow_mut().get("gitlab-domains"); - let domains = match gitlab_domains.as_array() { - Some(arr) => arr.clone(), - None => return false, - }; - let origin_in_domains = domains.values().any(|v| v.as_string() == Some(origin_url)); - let bc_in_domains = domains - .values() - .any(|v| v.as_string() == Some(bc_origin_url.as_str())); - if !origin_in_domains && !bc_in_domains { + if !in_array( + PhpMixed::String(origin_url.to_string()), + &gitlab_domains, + true, + ) && !in_array( + PhpMixed::String(bc_origin_url.clone()), + &gitlab_domains, + true, + ) { return false; } diff --git a/crates/shirabe/src/util/process_executor.rs b/crates/shirabe/src/util/process_executor.rs index ab4768a4..1f00eaeb 100644 --- a/crates/shirabe/src/util/process_executor.rs +++ b/crates/shirabe/src/util/process_executor.rs @@ -10,12 +10,14 @@ use shirabe_external_packages::composer::pcre::{CaptureKey, Preg}; use shirabe_external_packages::seld::signal::SignalHandler; use shirabe_external_packages::symfony::process::ExecutableFinder; use shirabe_external_packages::symfony::process::Process; +use shirabe_external_packages::symfony::process::ProcessMock; use shirabe_external_packages::symfony::process::exception::ProcessSignaledException; use shirabe_external_packages::symfony::process::exception::RuntimeException as SymfonyProcessRuntimeException; use shirabe_php_shim::{ - LogicException, PHP_EOL, PhpMixed, array_intersect, array_map, call_user_func, escapeshellarg, - explode, implode, in_array, is_array, is_dir, is_numeric, is_string, php_regex, rtrim, sprintf, - str_replace, strcspn, strlen, strpbrk, strtolower, strtr_array, substr_replace, trim, + LogicException, PHP_EOL, PhpMixed, RuntimeException, array_intersect, array_map, + call_user_func, escapeshellarg, explode, implode, in_array, is_array, is_dir, is_numeric, + is_string, php_regex, rtrim, sprintf, str_replace, strcspn, strlen, strpbrk, strtolower, + strtr_array, substr_replace, trim, }; use std::sync::{LazyLock, Mutex}; @@ -41,8 +43,9 @@ pub struct ProcessExecutor { /// @var bool allow_async: bool, /// Test-only mock state. `None` in production; set via [`ProcessExecutor::__expects`] in tests. - /// Mirrors `composer/tests/Composer/Test/Mock/ProcessExecutorMock.php`. - mock: Option, + /// Mirrors `composer/tests/Composer/Test/Mock/ProcessExecutorMock.php`. Wrapped in a `RefCell` + /// so `execute_async`'s `&self` receiver can still consume the expectation queue. + mock: Option>, } /// Test-only state for the ProcessExecutorMock behaviour (cf. @@ -154,7 +157,16 @@ impl ProcessExecutor { /// Convenience wrapper used by phase-A code that calls /// `process.execute(&[String], &mut String, Option<&str>) == 0`. - /// Forwards to `execute`, returning the status code (0 on Err for compatibility). + /// Forwards to `execute`, returning the status code (1 on Err for compatibility) — this + /// mirrors PHP call sites that check the `int` return of `execute()` without a surrounding + /// `try`/`catch`, where an uncaught mock-mismatch exception would otherwise propagate. + // TODO(phase-d): under a strict `ProcessExecutorMock`, an incomplete expectation list now + // surfaces here as a swallowed "exit code 1" instead of the old `panic!`, so a future test + // ported through this call site could silently take a wrong branch instead of failing loudly. + // `ProcessExecutorMockGuard::__assert_complete` still catches unconsumed expectations at + // scope exit, but not a mismatch that happened to consume nothing. Distinguishing "expectation + // mismatch" from "real process failure" here would need a marker type incompatible with + // `RuntimeException` (see `mock_match`'s doc comment) — deferred until a concrete test needs it. pub fn execute_args( &mut self, command: &[String], @@ -372,26 +384,22 @@ impl ProcessExecutor { .unwrap_or(0)) } - /// Mock replacement for `do_execute` when [`Self::mock`] is set (cf. - /// `ProcessExecutorMock::doExecute`). Logs the command, matches it against the head of the - /// expectation queue (exact `===`), pops on match (firing the optional callback), falls back to - /// the default handler in non-strict mode, or panics in strict mode. Emits stdout/stderr through - /// the output target and records `error_output`. - fn mock_do_execute<'o, O>( - &mut self, - command: PhpMixed, + /// Shared expectation-matching logic behind the mock branches of `do_execute` and + /// `execute_async` (cf. `ProcessExecutorMock::doExecute`). Logs the command, matches it + /// against the head of the expectation queue (exact `===`), pops on match (firing the + /// optional callback), falls back to the default handler in non-strict mode, or returns an + /// error in strict mode (cf. PHPUnit's `AssertionFailedError`, itself a catchable + /// `\RuntimeException` — callers such as `Git::get_mirror_default_branch` rely on being able + /// to catch a strict-mode mismatch rather than have it abort the process). Returns `(stdout, + /// stderr, return)`. Takes `&self`: the expectation queue is wrapped in a `RefCell` so + /// `execute_async`'s `&self` receiver can still consume it. + fn mock_match( + &self, + command: &PhpMixed, cwd: Option<&str>, - output: O, - ) -> anyhow::Result - where - O: IntoExecOutput<'o>, - { - let capture_output = output.capture_output(); - self.capture_output = capture_output; - self.error_output = String::new(); - - let command_string = if is_array(&command) { - match &command { + ) -> anyhow::Result<(String, String, i64)> { + let command_string = if is_array(command) { + match command { PhpMixed::List(l) => implode( " ", &l.iter() @@ -410,24 +418,23 @@ impl ProcessExecutor { command.as_string().unwrap_or("").to_string() }; - let mock = self.mock.as_mut().unwrap(); + let mut mock = self.mock.as_ref().unwrap().borrow_mut(); mock.log.push(command_string.clone()); let matched = mock .expectations .as_ref() - .map(|exps| !exps.is_empty() && exps[0].cmd == command) + .map(|exps| !exps.is_empty() && exps[0].cmd == *command) .unwrap_or(false); let (stdout, stderr, r#return); + let mut callback = None; if matched { let mut expect = mock.expectations.as_mut().unwrap().remove(0); stdout = expect.stdout.clone(); stderr = expect.stderr.clone(); r#return = expect.r#return; - if let Some(callback) = expect.callback.as_mut() { - callback(); - } + callback = expect.callback.take(); } else if !mock.strict { stdout = mock.default_handler.stdout.clone(); stderr = mock.default_handler.stderr.clone(); @@ -440,18 +447,57 @@ impl ProcessExecutor { .map(|exps| format!("Expected {:?} at this point.", exps[0].cmd)) .unwrap_or_else(|| "Expected no more calls at this point.".to_string()); let received = mock.log[..mock.log.len().saturating_sub(1)].join(PHP_EOL); - panic!( - "Received unexpected command {:?} in \"{}\"{}{}{}Received calls:{}{}", - command, - cwd.unwrap_or(""), - PHP_EOL, - expected, - PHP_EOL, - PHP_EOL, - received - ); + // PHPUnit's `AssertionFailedError` (thrown by `ProcessExecutorMock::doExecute` on a + // strict-mode mismatch) extends `\RuntimeException`, so PHP call sites that + // `catch (\RuntimeException $e)` around a mock-driven git/hg/svn call (e.g. + // `GitDriver::supports`) treat a mismatch as an ordinary recoverable failure. Using + // the same `RuntimeException` type here keeps `downcast_ref::()` + // checks working the same way against a mismatch. + return Err(RuntimeException { + message: format!( + "Received unexpected command {:?} in \"{}\"{}{}{}Received calls:{}{}", + command, + cwd.unwrap_or(""), + PHP_EOL, + expected, + PHP_EOL, + PHP_EOL, + received + ), + code: 0, + } + .into()); + } + + // Release the RefMut before firing the callback: a callback that re-enters the same + // executor (e.g. via a cloned `Rc>`) would otherwise hit + // `already mutably borrowed` here even before reaching the outer RefCell. + drop(mock); + if let Some(mut callback) = callback { + callback(); } + Ok((stdout, stderr, r#return)) + } + + /// Mock replacement for `do_execute` when [`Self::mock`] is set (cf. + /// `ProcessExecutorMock::doExecute`). Delegates the matching to [`Self::mock_match`], then + /// emits stdout/stderr through the output target and records `error_output`. + fn mock_do_execute<'o, O>( + &mut self, + command: PhpMixed, + cwd: Option<&str>, + output: O, + ) -> anyhow::Result + where + O: IntoExecOutput<'o>, + { + let capture_output = output.capture_output(); + self.capture_output = capture_output; + self.error_output = String::new(); + + let (stdout, stderr, r#return) = self.mock_match(&command, cwd)?; + // Feed stdout/stderr through the output target, mirroring the PHP `$callback(...)` calls. match output.to_callback() { Ok(mut callback) => { @@ -489,12 +535,12 @@ impl ProcessExecutor { strict: bool, default_handler: MockHandler, ) { - self.mock = Some(ProcessExecutorMockState { + self.mock = Some(std::cell::RefCell::new(ProcessExecutorMockState { expectations: Some(expectations), strict, default_handler, log: Vec::new(), - }); + })); } /// For testing only. Asserts all configured expectations were consumed (cf. @@ -503,6 +549,7 @@ impl ProcessExecutor { let Some(mock) = self.mock.as_ref() else { return; }; + let mock = mock.borrow(); // Not configured to expect anything, so no need to react here. let Some(expectations) = mock.expectations.as_ref() else { return; @@ -548,13 +595,18 @@ impl ProcessExecutor { /// starts a process on the commandline in async mode /// - /// Returns a future that does NOT borrow the executor: everything it needs is captured up + /// The returned future does NOT borrow the executor: everything it needs is captured up /// front, so callers can drop their `Ref`/`RefMut` on the shared `Rc>` - /// before awaiting (`let fut = pe.borrow().execute_async(...); fut.await`). Holding a borrow - /// across the await would panic as soon as a sibling future or a sync `execute()` call touches - /// the same executor. The max_jobs throttle is enforced by the semaphore. + /// before awaiting (`let fut = pe.borrow_mut().execute_async(...); fut.await`). Holding a + /// borrow across the await would panic as soon as a sibling future or a sync `execute()` call + /// touches the same executor. The max_jobs throttle is enforced by the semaphore. + /// + /// Takes `&mut self` (unlike the `&self` used while this only read `self.mock`) so the mock + /// branch can update `error_output`/`capture_output` before returning, mirroring + /// `ProcessExecutorMock::executeAsync` — which resolves through the same `doExecute` the sync + /// path uses, so it updates the executor's cached error output too (cf. `mock_do_execute`). pub fn execute_async( - &self, + &mut self, command: C, cwd: Option<&str>, ) -> std::pin::Pin>>> @@ -564,11 +616,22 @@ impl ProcessExecutor { let command = command.into_exec_command(); if self.mock.is_some() { // PHP resolves the promise with a Process mock whose getOutput/isSuccessful/getExitCode - // reflect the doExecute result. We cannot fabricate a Process mock here: Process has no - // test seam in the external-packages crate (out of scope to modify). No portable test - // currently exercises the async mock path, so leave it unimplemented rather than - // returning a misleading Process. - todo!("ProcessExecutorMock async path needs a Process mock seam in external-packages"); + // reflect the doExecute result; reuse the same expectation matching as the sync mock + // branch and fabricate the resolved Process via `Process::__mock`, a test seam mirroring + // `ZipArchive::__mock`. + let matched = self.mock_match(&command, cwd); + if let Ok((_, ref stderr, _)) = matched { + self.capture_output = true; + self.error_output = stderr.clone(); + } + return Box::pin(async move { + let (stdout, stderr, r#return) = matched?; + Ok(Process::__mock(ProcessMock { + exit_code: r#return, + stdout, + stderr, + })) + }); } let allow_async = self.allow_async; let semaphore = self.semaphore.clone(); -- cgit v1.3.1