Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TempDir: Provide an API to not delete-dir-on-drop without consuming the TempDir #31

Closed
vincentdephily opened this issue Oct 4, 2018 · 7 comments
Labels
enhancement Improve the expected

Comments

@vincentdephily
Copy link

vincentdephily commented Oct 4, 2018

The current API offers into_path(self) -> PathBuf which is nice and clean (not sure why PathBuf instead of Path, but I can live with that), but forces you to decide ahead of time if you want a temporary or persisted path.

I'd like to change the behaviour at runtime, but am hitting the following snags:

  • If I call into_path() near the end of my scope, it might not get called because of asserts that triggered earlyer (precisely when I'd have liked to keep the tempdir).
  • If I call it at the beginning of my scope, I either run into PathBuf/TempDir type mismatches, or into temporary value does not live long enough problems:
let tempdir = TempDir::new().unwrap();
// Doesn't compile because of lifetime issues
let cwd = match std::env::var("ASSERTFS_KEEP_TEMP") {
    Ok(_) => tempdir.into_path().as_path(),
    Err(_) => tempdir.path(),
};
assert!(dostuff(cwd).is_ok());
// Too late: assert has fired already.
if std::env::var("ASSERTFS_KEEP_TEMP").is_ok() {
    tempdir.into_path();
}

Perhaps I missed an easy solution, but I doubt it would be as easy as having some kind of delete_on_drop(bool) API:

let tempdir = TempDir::new()
                      .unwrap()
                      .delete_on_drop(std::env::var("ASSERTFS_KEEP_TEMP").is_ok());
assert!(dostuff(cwd).is_ok());

Workaround

let tempdir = TempDir::new().unwrap();
let mut cwd = PathBuf::new();
match env::var("ASSERTFS_KEEP_TEMP") {
    Ok(_) => cwd.push(&tempdir.into_path()),
    Err(_) => cwd.push(tempdir.path()),
}
assert!(dostuff(cwd).is_ok());
@epage
Copy link
Contributor

epage commented Oct 4, 2018

Could you describe the motivation for keeping the temp dir?

@vincentdephily
Copy link
Author

I'm running unit tests that create files in the tempdir. Most of the time I want them deleted, but if tests start failing I want to look at the files to figure out what happened.

Even outside of unittesting, there are many tools that implement some kind of --keep-temp-files option, so this seems to me like a fairly common need.

@vincentdephily
Copy link
Author

For what it's worth, I eventually managed to make my usecase work with the current API:

let tempdir = TempDir::new().unwrap();
let mut cwd = PathBuf::new();
match env::var("ASSERTFS_KEEP_TEMP") {
    Ok(_) => cwd.push(&tempdir.into_path()),
    Err(_) => cwd.push(tempdir.path()),
}
assert!(dostuff(cwd).is_ok());

It works but it's not elegant (copying the path like this seems like a hack), and took me a while to come up with. An obvious TempDir API to do the same would still be welcome.

@epage
Copy link
Contributor

epage commented Oct 4, 2018

Thanks for describing that.

For tests, that hasn't been as important to me because they have been things I could always exercise with the [bin] target but that isn't true in all cases.

@epage epage added the enhancement Improve the expected label Oct 4, 2018
@epage
Copy link
Contributor

epage commented Oct 4, 2018

I think another important aspect for the user, whether its done inside of assert_fs or the tests, is to print the path in case the failure message doesn't have it.

@vincentdephily
Copy link
Author

Yes, I wouldn't mind replacing my println!("{:?}", tempdir.path()) with println!("{}", tempdir)... but that's a different issue ;)

@epage
Copy link
Contributor

epage commented Jan 27, 2019

I think I'm going to give making a newtype for TempDir a try which gives me more flexibility in implementing this idea.

epage added a commit to epage/assert_fs that referenced this issue Jan 29, 2019
@epage epage closed this as completed in 82220c8 Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

2 participants