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/tests/command/init_command_test.rs | 23 ++- crates/shirabe/tests/common/test_case.rs | 41 ++++++ .../tests/downloader/zip_downloader_test.rs | 163 +++++++++++++++++---- .../event_dispatcher/event_dispatcher_test.rs | 32 +++- .../package/loader/root_package_loader_test.rs | 12 +- .../tests/package/version/version_guesser_test.rs | 3 - .../tests/repository/vcs/github_driver_test.rs | 13 +- 7 files changed, 238 insertions(+), 49 deletions(-) (limited to 'crates/shirabe/tests') diff --git a/crates/shirabe/tests/command/init_command_test.rs b/crates/shirabe/tests/command/init_command_test.rs index 9c52dddd..57777e35 100644 --- a/crates/shirabe/tests/command/init_command_test.rs +++ b/crates/shirabe/tests/command/init_command_test.rs @@ -1,9 +1,10 @@ //! ref: composer/tests/Composer/Test/Command/InitCommandTest.php -use crate::test_case::{RunOptions, get_application_tester, init_temp_composer}; +use crate::test_case::{RestoreEnv, RunOptions, get_application_tester, init_temp_composer}; use serial_test::serial; use shirabe::command::init_command::InitCommand; use shirabe::json::JsonFile; +use shirabe::util::platform::Platform; use shirabe_php_shim::{PHP_SERVER, PhpMixed}; use tempfile::TempDir; @@ -597,13 +598,27 @@ fn test_format_authors() { assert_eq!(expected, authors[0]); } +/// ref: InitCommandTest::testGetGitConfig. +/// +/// Composer's own CI runs `git config --global user.name/user.email` before the test suite so a +/// global config is guaranteed to exist; here `HOME` is pointed at a throwaway directory carrying +/// its own `.gitconfig` with those keys so the test doesn't depend on (or mutate) the real user's +/// global git config. #[test] -#[ignore = "requires the host's global git config to have user.name/user.email set (Composer's \ - own CI runs `git config --global user.name/user.email` before the test suite); \ - fails in environments without that global config"] +#[serial] fn test_get_git_config() { set_up(); + let home = TempDir::new().unwrap(); + std::fs::write( + home.path().join(".gitconfig"), + "[user]\n\tname = Test User\n\temail = test-user@example.com\n", + ) + .unwrap(); + let original_home = Platform::get_env("HOME"); + Platform::put_env("HOME", &home.path().to_string_lossy()); + let _restore_home = RestoreEnv::new("HOME", original_home); + let command = InitCommand::new(); let git_config = command.__get_git_config(); assert!(git_config.contains_key("user.name")); diff --git a/crates/shirabe/tests/common/test_case.rs b/crates/shirabe/tests/common/test_case.rs index 6c3bb5ad..7f8e1d54 100644 --- a/crates/shirabe/tests/common/test_case.rs +++ b/crates/shirabe/tests/common/test_case.rs @@ -16,6 +16,7 @@ use shirabe::package::handle::{ CompleteAliasPackageHandle, CompletePackageHandle, PackageInterfaceHandle, }; use shirabe::repository::{InstalledFilesystemRepository, WritableRepositoryInterface}; +use shirabe::util::Git as GitUtil; use shirabe::util::http_downloader::HttpDownloader; use shirabe::util::r#loop::Loop; use shirabe::util::platform::Platform; @@ -168,6 +169,46 @@ pub fn init_temp_composer( } } +/// For testing only. Resets the cached `GitUtil` version static on drop, mirroring the +/// `ReflectionProperty(GitUtil::class, 'version')->setValue(null, false)` reset done in +/// `VersionGuesserTest`'s setUp/tearDown. `GitUtil::VERSION` is shared process-wide, so any test +/// that seeds it via `GitUtil::__set_version` should hold one of these (and be `#[serial]`, since +/// `#[serial]` only excludes other `#[serial]` tests from running concurrently). +pub struct GitVersionGuard; + +impl Drop for GitVersionGuard { + fn drop(&mut self) { + GitUtil::__reset_version(); + } +} + +/// Restores an environment variable to its prior value (or clears it if it was unset) once +/// dropped. Pair with `Platform::get_env(name)` captured before the override: +/// ```ignore +/// let original = Platform::get_env("HOME"); +/// Platform::put_env("HOME", "/tmp/fake-home"); +/// let _restore = RestoreEnv::new("HOME", original); +/// ``` +pub struct RestoreEnv { + name: &'static str, + original: Option, +} + +impl RestoreEnv { + pub fn new(name: &'static str, original: Option) -> Self { + Self { name, original } + } +} + +impl Drop for RestoreEnv { + fn drop(&mut self) { + match &self.original { + Some(value) => Platform::put_env(self.name, value), + None => Platform::clear_env(self.name), + } + } +} + fn null_io() -> std::rc::Rc> { std::rc::Rc::new(std::cell::RefCell::new(NullIO::new())) } diff --git a/crates/shirabe/tests/downloader/zip_downloader_test.rs b/crates/shirabe/tests/downloader/zip_downloader_test.rs index 593f3e76..338b2b54 100644 --- a/crates/shirabe/tests/downloader/zip_downloader_test.rs +++ b/crates/shirabe/tests/downloader/zip_downloader_test.rs @@ -14,6 +14,7 @@ use shirabe::util::HttpDownloader; use shirabe::util::ProcessExecutor; use shirabe::util::filesystem::Filesystem; use shirabe::util::r#loop::Loop; +use shirabe::util::process_executor::MockHandler; use shirabe_php_shim::{PhpMixed, ZipArchive, ZipArchiveMock}; use shirabe_semver::VersionParser; use tempfile::TempDir; @@ -87,10 +88,17 @@ impl Drop for TearDown { } fn make_downloader(set_up: &SetUp) -> ZipDownloader { - let filesystem = std::rc::Rc::new(std::cell::RefCell::new(Filesystem::new(None))); let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(Some( set_up.io.clone(), )))); + make_downloader_with_process(set_up, process) +} + +fn make_downloader_with_process( + set_up: &SetUp, + process: std::rc::Rc>, +) -> ZipDownloader { + let filesystem = std::rc::Rc::new(std::cell::RefCell::new(Filesystem::new(None))); ZipDownloader::new( set_up.io.clone(), set_up.config.clone(), @@ -102,11 +110,6 @@ fn make_downloader(set_up: &SetUp) -> ZipDownloader { ) } -// The system-unzip / non-windows-fallback paths route through ProcessExecutor::execute_async, whose -// mock branch is an unimplemented todo!() (no Process mock seam exists in the external-packages -// crate). The PHP tests below mock Process/ProcessExecutor::executeAsync, which is not reproducible -// here, so they remain ignored. -// // testErrorMessages drives a real HttpDownloader + Loop, but RemoteFilesystem::get_remote_contents // is a phase-c stub returning None, so the file:// dist download fails before the ZipArchive path. @@ -266,38 +269,146 @@ fn test_zip_archive_only_good() { result.expect("extract should succeed"); } -#[ignore = "routes through ProcessExecutor::execute_async whose mock branch is todo!() (no Process mock seam in external-packages)"] +// setPrivateProperty('unzipCommands', [['unzip', 'unzip -qq %s -d %s']]) in PHP: a single +// two-element commandSpec (executable name, then one literal arg string that contains %s +// placeholders rather than %file%/%path%, so it is passed through to executeAsync verbatim). The +// PHPUnit test fully replaces $processExecutor, so the exact command content is never asserted on; +// this only needs to be non-empty so extractWithSystemUnzip proceeds past the "no commands" +// short-circuit into ZipDownloader::extract_with_zip_archive. +fn unzip_command_spec() -> Vec> { + vec![vec!["unzip".to_string(), "unzip -qq %s -d %s".to_string()]] +} + #[test] +#[serial] fn test_system_unzip_only_failed() { - let _ = set_up(); - // TODO(phase-d): routes through ProcessExecutor::execute_async, whose mock branch is - // todo!() (no Process mock seam exists in the external-packages crate). - todo!() + let set_up = set_up(); + let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf()); + + ZipDownloader::__set_is_windows(Some(false)); + ZipDownloader::__set_has_zip_archive(Some(false)); + ZipDownloader::__set_unzip_commands(Some(unzip_command_spec())); + + let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(None))); + process.borrow_mut().__expects( + vec![], + false, + MockHandler { + r#return: 1, + stdout: String::new(), + stderr: "output".to_string(), + }, + ); + let downloader = make_downloader_with_process(&set_up, process); + + let filename = set_up.filename.to_string_lossy().into_owned(); + let result = run(downloader.extract(set_up.package.clone(), &filename, "vendor/dir")); + + let e = result.expect_err("expected RuntimeException"); + assert!( + e.to_string() + .contains("Failed to extract test/pkg: (1) unzip"), + "got: {e}" + ); } -#[ignore = "routes through ProcessExecutor::execute_async whose mock branch is todo!() (no Process mock seam in external-packages)"] #[test] +#[serial] fn test_system_unzip_only_good() { - let _ = set_up(); - // TODO(phase-d): routes through ProcessExecutor::execute_async, whose mock branch is - // todo!() (no Process mock seam exists in the external-packages crate). - todo!() + let set_up = set_up(); + let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf()); + + ZipDownloader::__set_is_windows(Some(false)); + ZipDownloader::__set_has_zip_archive(Some(false)); + ZipDownloader::__set_unzip_commands(Some(unzip_command_spec())); + + let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(None))); + process.borrow_mut().__expects( + vec![], + false, + MockHandler { + r#return: 0, + stdout: String::new(), + stderr: "output".to_string(), + }, + ); + let downloader = make_downloader_with_process(&set_up, process); + + let filename = set_up.filename.to_string_lossy().into_owned(); + let result = run(downloader.extract(set_up.package.clone(), &filename, "vendor/dir")); + + result.expect("extract should succeed"); } -#[ignore = "routes through ProcessExecutor::execute_async whose mock branch is todo!() (no Process mock seam in external-packages)"] #[test] +#[serial] fn test_non_windows_fallback_good() { - let _ = set_up(); - // TODO(phase-d): routes through ProcessExecutor::execute_async, whose mock branch is - // todo!() (no Process mock seam exists in the external-packages crate). - todo!() + let set_up = set_up(); + let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf()); + + ZipDownloader::__set_is_windows(Some(false)); + ZipDownloader::__set_has_zip_archive(Some(true)); + ZipDownloader::__set_unzip_commands(Some(unzip_command_spec())); + + let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(None))); + process.borrow_mut().__expects( + vec![], + false, + MockHandler { + r#return: 1, + stdout: String::new(), + stderr: "output".to_string(), + }, + ); + let downloader = make_downloader_with_process(&set_up, process); + let zip_archive = ZipArchive::__mock(ZipArchiveMock { + open: Ok(()), + count: 0, + extract_to: Ok(true), + }); + downloader.__set_zip_archive_object(Some(zip_archive)); + + let filename = set_up.filename.to_string_lossy().into_owned(); + let result = run(downloader.extract(set_up.package.clone(), &filename, "vendor/dir")); + + result.expect("extract should succeed"); } -#[ignore = "routes through ProcessExecutor::execute_async whose mock branch is todo!() (no Process mock seam in external-packages)"] #[test] +#[serial] fn test_non_windows_fallback_failed() { - let _ = set_up(); - // TODO(phase-d): routes through ProcessExecutor::execute_async, whose mock branch is - // todo!() (no Process mock seam exists in the external-packages crate). - todo!() + let set_up = set_up(); + let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf()); + + ZipDownloader::__set_is_windows(Some(false)); + ZipDownloader::__set_has_zip_archive(Some(true)); + ZipDownloader::__set_unzip_commands(Some(unzip_command_spec())); + + let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(None))); + process.borrow_mut().__expects( + vec![], + false, + MockHandler { + r#return: 1, + stdout: String::new(), + stderr: "output".to_string(), + }, + ); + let downloader = make_downloader_with_process(&set_up, process); + let zip_archive = ZipArchive::__mock(ZipArchiveMock { + open: Ok(()), + count: 0, + extract_to: Ok(false), + }); + downloader.__set_zip_archive_object(Some(zip_archive)); + + let filename = set_up.filename.to_string_lossy().into_owned(); + let result = run(downloader.extract(set_up.package.clone(), &filename, "vendor/dir")); + + let e = result.expect_err("expected RuntimeException"); + assert!( + e.to_string() + .contains("There was an error extracting the ZIP file"), + "got: {e}" + ); } diff --git a/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs b/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs index 2c38ae6a..0f392d7e 100644 --- a/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs +++ b/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs @@ -405,10 +405,34 @@ fn test_dispatcher_outputs_command() { } #[test] -#[ignore = "uses an unmocked ProcessExecutor running a real `exit 1`; depends on real shell execution"] +#[serial] fn test_dispatcher_outputs_error_on_failed_command() { let _tear_down = TearDown; - // TODO(phase-d): uses an unmocked ProcessExecutor running a real `exit 1`; depends on real - // shell execution - todo!() + + let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(None))); + let composer = create_composer_instance(); + let io = std::rc::Rc::new(std::cell::RefCell::new( + BufferIO::new(String::new(), output_interface::VERBOSITY_NORMAL, None).unwrap(), + )); + let io_dyn: std::rc::Rc> = io.clone(); + + let code = "exit 1"; + let mut dispatcher = + dispatcher_with_listeners(&composer, io_dyn, process, listeners_const(vec![code])); + + let result = dispatcher.dispatch_script( + ScriptEvents::POST_INSTALL_CMD, + false, + vec![], + IndexMap::new(), + ); + + let e = result.expect_err("expected ScriptExecutionException"); + assert!(e.to_string().contains("Error Output: "), "got: {e}"); + + let expected = format!( + "> exit 1{eol}Script exit 1 handling the post-install-cmd event returned with error code 1{eol}", + eol = PHP_EOL + ); + assert_eq!(expected, io.borrow().get_output()); } diff --git a/crates/shirabe/tests/package/loader/root_package_loader_test.rs b/crates/shirabe/tests/package/loader/root_package_loader_test.rs index 069cf5a7..248c4e6a 100644 --- a/crates/shirabe/tests/package/loader/root_package_loader_test.rs +++ b/crates/shirabe/tests/package/loader/root_package_loader_test.rs @@ -5,6 +5,7 @@ // look-around regex the regex crate cannot compile. use crate::process_executor_mock::{cmd, cmd_full, get_process_executor_mock}; +use crate::test_case::GitVersionGuard; use indexmap::IndexMap; use serial_test::serial; use shirabe::config::Config; @@ -72,16 +73,6 @@ fn require_map(entries: &[(&str, &str)]) -> PhpMixed { PhpMixed::Array(m) } -// Resets the cached git `version` static on drop so a seeded value does not leak into other -// tests in this binary (VersionGuesserTest seeds/resets the same static). -struct GitVersionGuard; - -impl Drop for GitVersionGuard { - fn drop(&mut self) { - GitUtil::__reset_version(); - } -} - // A test double for the concrete VersionGuesser, supplied through the VersionGuesserInterface seam. mockall::mock! { #[derive(Debug)] @@ -271,7 +262,6 @@ fn test_pretty_version_for_root_package_in_version_branch() { } #[test] -#[ignore = "feature-branch guessing calls ProcessExecutor::execute_async, whose mock path is todo!()"] #[serial] fn test_feature_branch_pretty_version() { // proc_open() is always available; the PHP markTestSkipped guard does not apply here. diff --git a/crates/shirabe/tests/package/version/version_guesser_test.rs b/crates/shirabe/tests/package/version/version_guesser_test.rs index 92d5e90b..dd71d8ab 100644 --- a/crates/shirabe/tests/package/version/version_guesser_test.rs +++ b/crates/shirabe/tests/package/version/version_guesser_test.rs @@ -185,7 +185,6 @@ fn test_guess_version_does_not_see_custom_default_branch_as_non_feature_branch() assert_eq!(another_commit_hash, version_data.commit.unwrap()); } -#[ignore = "feature-branch guessing calls ProcessExecutor::execute_async, whose mock path is todo!()"] #[test] #[serial] fn test_guess_version_reads_and_respects_non_feature_branches_configuration_for_arbitrary_naming() { @@ -239,7 +238,6 @@ fn test_guess_version_reads_and_respects_non_feature_branches_configuration_for_ ); } -#[ignore = "feature-branch guessing calls ProcessExecutor::execute_async, whose mock path is todo!()"] #[test] #[serial] fn test_guess_version_reads_and_respects_non_feature_branches_configuration_for_arbitrary_naming_regex() @@ -560,7 +558,6 @@ fn test_numeric_branches_show_nicely() { assert_eq!("1.5.9999999.9999999-dev", version_data.version.unwrap()); } -#[ignore = "remote-branch feature guessing calls ProcessExecutor::execute_async, whose mock path is todo!()"] #[test] #[serial] fn test_remote_branches_are_selected() { diff --git a/crates/shirabe/tests/repository/vcs/github_driver_test.rs b/crates/shirabe/tests/repository/vcs/github_driver_test.rs index 5b8367c7..8b30a796 100644 --- a/crates/shirabe/tests/repository/vcs/github_driver_test.rs +++ b/crates/shirabe/tests/repository/vcs/github_driver_test.rs @@ -5,12 +5,15 @@ use crate::io_stub::IOStub; use crate::process_executor_mock::{ ProcessExecutorMockGuard, cmd, cmd_full, get_process_executor_mock, }; +use crate::test_case::GitVersionGuard; use indexmap::IndexMap; +use serial_test::serial; use shirabe::config::Config; use shirabe::config::ConfigSourceInterface; use shirabe::io::IOInterface; use shirabe::io::null_io::NullIO; use shirabe::repository::vcs::GitHubDriver; +use shirabe::util::Git as GitUtil; use shirabe::util::filesystem::Filesystem; use shirabe::util::http_downloader::HttpDownloaderMockHandler; use shirabe::util::process_executor::{MockHandler, ProcessExecutor}; @@ -681,9 +684,17 @@ fn test_public_repository_archived() { assert_eq!(Some(true), data.get("abandoned").and_then(|v| v.as_bool())); } +// GitDriver::initialize calls GitUtil::cleanEnv, which calls GitUtil::getVersion; PHP's +// `GitUtil::$version` is a class-level static that persists for the whole PHPUnit run, so by the +// time this test runs it has already been populated (as a side effect of some earlier-run test +// invoking the real `git --version`) and the mock expectation list below never needs to include it. +// Rust test execution order isn't guaranteed the same way, so the cache is seeded explicitly here. #[test] -#[ignore = "GitDriver clone-fallback path runs an unexpected `git --version` (Git::get_version) not in the PHP mock expectation list; needs the version static seeded and the Rust sync_mirror command sequence to match"] +#[serial] fn test_private_repository_no_interaction() { + GitUtil::__set_version(Some("2.52.0".to_string())); + let _git_guard = GitVersionGuard; + let SetUp { home, config } = set_up(); let _tear_down = TearDown::new(home.path().to_path_buf()); -- cgit v1.3.1