Make sure rust-scripts are always executed in the final destination.#2414
Make sure rust-scripts are always executed in the final destination.#2414
Conversation
There was a problem hiding this comment.
Pull request overview
Updates gix-testtools fixture helpers so Rust-based fixture/post-processing closures can be invoked more flexibly, enabling results to be computed against the final writable destination rather than only against cached/read-only locations.
Changes:
- Switches various fixture “post” closures from
FnOncetoFnMutand threads them through as&mutwhere needed. - Adjusts
rust_fixture_writable_*(CopyFromReadOnly path) to re-run the closure on the final tempdir to “prime” the returned value for the writable location. - Refactors scripted fixture internals to pass post-process closures by mutable reference.
| )?; | ||
| copy_recursively_into_existing_dir(ro_dir, dst.path())?; | ||
| (dst, post_result) | ||
| (dst, _res_ignored) |
There was a problem hiding this comment.
In Creation::CopyFromReadOnly, the returned Option<T> comes from scripted_fixture_read_only_with_args_inner(...), so any T computed by post_process is based on the read-only directory, not on the returned writable dst. With the new FnMut/as_mut() plumbing, consider re-running the post-process closure on dst after copy_recursively_into_existing_dir(...) (e.g. with FixtureState::Fresh(dst.path())) and returning that value instead; otherwise callers can receive paths that don’t exist in the writable tempdir.
| (dst, _res_ignored) | |
| // Re-run post_process on the writable destination to ensure `T` matches `dst` | |
| let post_result = if let Some((_version, ref mut f)) = post_process { | |
| Some(f(FixtureState::Fresh(dst.path()))?) | |
| } else { | |
| None | |
| }; | |
| (dst, post_result) |
| pub fn scripted_fixture_read_only_with_post<T>( | ||
| script_name: impl AsRef<Path>, | ||
| version: u32, | ||
| post_process: impl FnOnce(FixtureState<'_>) -> PostResult<T>, | ||
| post_process: impl FnMut(FixtureState<'_>) -> PostResult<T>, | ||
| ) -> Result<(PathBuf, T)> { |
There was a problem hiding this comment.
Changing the post_process bound from FnOnce to FnMut for the read-only *_with_post APIs is a breaking and more restrictive public API change for gix-testtools (it rejects closures that consume captures). If you only need multiple invocations for writable fixtures, consider keeping the read-only variants (and scripted_fixture_read_only_with_args_inner) accepting FnOnce and pass &mut closures from writable code as needed.
| /// It may be called multiple times, and the returned `T` will be primed on the final, writable location. | ||
| /// | ||
| /// `version` should be incremented when the closure's behavior changes to invalidate the cache. | ||
| /// `name` is used to identify this fixture for caching purposes and should be unique within the crate. |
There was a problem hiding this comment.
rust_fixture_writable still takes a mode: Creation parameter, but the docs no longer mention what mode controls (the earlier “mode controls how…” line was removed). Please re-add a brief explanation of mode here so callers understand the CopyFromReadOnly vs ExecuteScript tradeoff.
| /// `name` is used to identify this fixture for caching purposes and should be unique within the crate. | |
| /// `name` is used to identify this fixture for caching purposes and should be unique within the crate. | |
| /// `mode` controls how the writable directory is prepared: [`Creation::CopyFromReadOnly`] copies a cached | |
| /// read-only fixture into a fresh temporary directory, while [`Creation::ExecuteScript`] reruns the closure | |
| /// to build the fixture from scratch each time. |
| let (ro_dir, _res_ignored) = rust_fixture_read_only_inner(name, version, &mut make_fixture, None, root)?; | ||
| copy_recursively_into_existing_dir(ro_dir, dst.path())?; | ||
| let res = make_fixture(FixtureState::Fresh(dst.path()))?; | ||
| res |
There was a problem hiding this comment.
Test coverage: the new behavior in rust_fixture_writable_inner for Creation::CopyFromReadOnly intentionally calls make_fixture again on the final tempdir to “prime” the returned T for the writable location. Please add a regression test that returns something path-dependent (e.g. a PathBuf into the fixture) and asserts it points into tmp.path() rather than the cached read-only directory.
| pub fn scripted_fixture_writable_with_post<T>( | ||
| script_name: impl AsRef<Path>, | ||
| version: u32, | ||
| post_process: impl FnOnce(FixtureState<'_>) -> PostResult<T>, | ||
| post_process: impl FnMut(FixtureState<'_>) -> PostResult<T>, | ||
| ) -> Result<(tempfile::TempDir, T)> { |
There was a problem hiding this comment.
The doc comment immediately above scripted_fixture_writable_with_post mentions a FixtureState::Cached variant and says Fresh is “newly created”, but FixtureState only has Uninitialized and Fresh (where Fresh corresponds to already-created/cached). Please adjust the wording to match the actual enum variants/semantics so callers know when it’s safe to mutate the fixture directory.
1 participant
An oversight, and still no test for it.
Nonetheless, I think it's way more testable now and also overall better tested than before.