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

What is needed for 1.0? #46

Closed
epage opened this issue Mar 22, 2019 · 10 comments
Closed

What is needed for 1.0? #46

epage opened this issue Mar 22, 2019 · 10 comments
Labels
question Uncertainty is involved

Comments

@epage
Copy link
Contributor

epage commented Mar 22, 2019

Collecting feedback or assert_fs. Please post even if you think it might not be needed for 1.0 so we can try to get the big picture of uses, challenges, etc. We're also looking at including experience reports in the 1.0 announcement blog post.

Preferably, please update to the latest version first but don't let upgrading stop you from providing feedback

To help spur feedback, consider these questions

  • Why do you use this crate?
  • Why don't you use it more?
  • How does this crate fit into your testing strategy?
  • Is there anything that feels out of place / not rust-like?
  • Are there common patterns you implement on top of this crate (e.g. fixture setup or assertions we don't support)?

Summary

Areas to improve

Successes

@volks73

I use assert_fs because I wanted more elegant, shorter, and fluent-style tests. I liked how the crate appeared to make working with paths easier. I noticed a lot of my tests were just building paths to files and directories, which can be verbose and cumbersome in Rust. In an effort to reduce boilerplate and make my tests shorter and easier to follow, I started using assert_fs.

@epage epage added the question Uncertainty is involved label Mar 22, 2019
@epage
Copy link
Contributor Author

epage commented Mar 22, 2019

@Valodim, @glehmann, @vincentdephily, and @mssun: you all have participated in some issues. Could you provide input on this?

@epage epage pinned this issue Mar 22, 2019
@passcod

This comment has been minimized.

@epage

This comment has been minimized.

@volks73
Copy link

volks73 commented Mar 24, 2019

My main usage of assert_fs is testing a cargo subcommand, cargo-wix. I need to create temporary directories, one-off cargo projects, check the existence of files, check the validity and contents of the files, and do this on Windows.

I use assert_fs because I wanted more elegant, shorter, and fluent-style tests. I liked how the crate appeared to make working with paths easier. I noticed a lot of my tests were just building paths to files and directories, which can be verbose and cumbersome in Rust. In an effort to reduce boilerplate and make my tests shorter and easier to follow, I started using assert_fs.

I would use assert_fs more if I could upgrade from v0.9. The assert_fs crate wraps the tempfile crate, but does not expose all of the functionality. For example, I need to be able to define a prefix for a temporary directory. The default .tmp prefix is an invalid name for a Cargo package. Windows is also not too keen on file and folder names with leading periods. This means I need to use the tempfile::Builder type to add a custom prefix. There is no From<tempfile::TempDir> trait implementation for assert_fs::TempDir, so I cannot use the tempfile::Builder type to customize a temporary directory and then "cast" it to the assert_fs::TempDir type to gain the assertion-ness and conveniences from the assert_fs crate, nor is there a similar Builder interface available. Thus, I am stuck using v0.9 of assert_fs, which does not wrap tempfile and instead just extends it.

If I try to bump to v0.11, I get a bunch of:

no method named child found for type tempfile::TempDir...

errors. I realize now, I should probably submit an issue for this, but it is an example of a common pattern or option I implement for my tests. Maybe I am just missing something on how to use temporary directories in relation to the assert_fs crate?

The above anecdote highlights an inconsistency between the two sibling crates: assert_fs and assert_cmd which are often mentioned in the same breath. The assert_cmd extends the standard library's Command type with traits, i.e. adds assertion features. The assert_fs crate in v0.11 appears to be following more of a "composition"-themed implementation instead of an extension-themed implementation, which is perfectly rust-like, but it is inconsistent with the extension-themed implementation of the assert_cmd. The two crates can have differently themed implementations since they are targeted at testing different systems, but I came to use this crate after a pleasant experience using the assert_cmd crate in another project; thus, I was expecting some consistency.

If going with the composition-themed implementation, then methods and functions to upcast/downcast to the inner type, which is rust-like, would be greatly appreciated. Examples of using both crates together would be helpful, too.

@epage
Copy link
Contributor Author

epage commented Mar 25, 2019

@volks73 thank you for taking the time to write up that feedback!

I would use assert_fs more if I could upgrade from v0.9. The assert_fs crate wraps the tempfile crate, but does not expose all of the functionality. For example, I need to be able to define a prefix for a temporary directory. The default .tmp prefix is an invalid name for a Cargo package. Windows is also not too keen on file and folder names with leading periods. This means I need to use the tempfile::Builder type to add a custom prefix. There is no Fromtempfile::TempDir trait implementation for assert_fs::TempDir, so I cannot use the tempfile::Builder type to customize a temporary directory and then "cast" it to the assert_fs::TempDir type to gain the assertion-ness and conveniences from the assert_fs crate, nor is there a similar Builder interface available. Thus, I am stuck using v0.9 of assert_fs, which does not wrap tempfile and instead just extends it.

So first, a workaround. You can create a ChildPath directly, giving you access to all of assert_fss API without using its own temp dir support. The downside is that makes it annoying for how you are using assert_fs because create_test_package returns a TempDir and then in each test, you'd need to construct a ChildPath. Created #48 to have this documented.

As I'll mention below, I made the assumption that custom prefix/suffix and other functionality on the builder wasn't needed within the confines of testing. I assumed people would just create their needed directory or file within the temp dir. It appears I was wrong and I want to better understand the requirements going into how I was wrong so I can ensure any future evolution meets your needs.

Looking at the tests, it seems the tests do a cargo init inside a temp dir with a prefix. This makes it so the dir has a valid cargo name and you use that name for your project under test.

Is there a reason that this is done rather than passing --name <name> to cargo init?

The assert_fs crate in v0.11 appears to be following more of a "composition"-themed implementation instead of an extension-themed implementation, which is perfectly rust-like, but it is inconsistent with the extension-themed implementation of the assert_cmd. The two crates can have differently themed implementations since they are targeted at testing different systems, but I came to use this crate after a pleasant experience using the assert_cmd crate in another project; thus, I was expecting some consistency.

(So leaving this background for last since it doesn't really help you.)

Yeah, I can see the divergent API styles for assert_fs and assert_cmd throwing people off.

I switched to newtypes in #41. I wish I had recorded my reasons. My guesses for how the two APis ended up diverging:

  • std::process::Command is stable while tempdir was not. tempfile is but they have had 3 major versions so far. We'd need to be in block step with whatever version of tempfile people are using.
  • Extending other crates is less than ideal
    • It was easy to not re-export "enough". I remember using assert_fs and running into issues of missing parts of the API that I needed, I think it was an error type. I had to add a dependency until I fixed assert_fs
    • It is harder to browse documentation for extension methods for foreign types, so avoiding them where possible is a big help. You can't just look at Command and see how we extend it. You have to browse the individual extension traits, once you know what all exists and where. This is much easier with a newtype since you can see what all functionality we embed on it directly and what all assert_fs traits it implements.
    • On assert_cmd, I got complaints about having to deal with too many different error types. I slightly improved that there but this helped but newtypes helped a lot with assert_fs.
  • There was a lot of interest in having a flexible API with assert_cmd while I assumed we only needed a subset of tempfile behavior,
    • There was interest in using assert_cmd with other process APIs, like duct
    • Pick and choose subsets of assert_cmd's functionality
  • asssert_fs had special requirements that was going to require custom types anyways

@volks73
Copy link

volks73 commented Mar 26, 2019

Is there a reason that this is done rather than passing --name to cargo init?

It was a while ago, and like you, I wish I had made better notes on my reasons. I can think of three possibilities:

  1. I guessed the majority of my users would be using the cargo init command without the --name <name> option as this is the default behavior. Thus, I wanted at least one test that would execute the default series of commands a user would. This happened to be the first test I wrote (default_works), which immediately encountered the prefix naming error. I resolved the error and then wrapped the solution in the create_test_package function, which I then re-used for all subsequent tests. The --name <name> option would work for every test except the "default", which I could have just implemented the "one-off" solution.
  2. I vaguely remember there being some problem with the --name <name> option as well. It may have been related to Windows Explorer (shell) not really work well with folder names containing a leading period. It is possible for applications and cmd.exe to create and remove folders with leading periods, but Windows Explorer has issues. I remember being a little annoyed with the tempfile crate for using a leading period on Windows, but I know it is needed on UNIX-like systems to hide files and folders. To avoid all of this, I just used the prefix option on the Builder. It is also possible that I was having some trouble when testing package configuration against paths and I may have assumed the two would be the same, which reminds to create a some tests and try the --name <name> option with cargo-wix.
  3. I forgot about the --name <name> option at the time I came up with the solution outline in 1.

I do wonder how common it is to create and test cargo subcommands within the Rust CLI community? My usage of assert_fs has only been on testing my cargo subcommand, but I do plan on using it the future for other, non-cargo-related, applications.

The reasons for the divergence between assert_cmd and assert_fs all make perfect sense. As mentioned, the two crates target different problems and needs. Thank you for sharing.

On a side note, I am just now seeing your comments to my gist on adding some STDIN test functionality to assert_cmd. I am sorry I did not reply to your comments and questions earlier.

@epage
Copy link
Contributor Author

epage commented Mar 26, 2019

Thanks for the reply!

t may have been related to Windows Explorer (shell) not really work well with folder names containing a leading period. It is possible for applications and cmd.exe to create and remove folders with leading periods, but Windows Explorer has issues

Thats strange. So far I've not seen this.

I do wonder how common it is to create and test cargo subcommands within the Rust CLI community?

I'm slowly adding tests to cargo-release that use assert_fs.

btw once I finish up the bare minimum for running cargo-release in a workspace, I plan to create a clap-cargo crate that has ready-to-use flags to embed in cargo subcommands to help people better mimic the built-in subcommands. I'll be using this to write -all support for cargo-release.

@vincentdephily
Copy link

I went thru my use of assert_fs, but nothings sticks out anymore. I always start with

        let mut sb = String::new();
        let f = tmpdir.child(c.file);
        let path = f.path();
        File::open(path).expect(&format!("Couldn't open {:?}", path))
                        .read_to_string(&mut sb)
                        .expect(&format!("Couldn't read {:?} as utf8", path));

which would be nice to a a convenience function for (TempDir.child_to_string() or something), but maybe that's too specific.

I also often test the file content with a regexp, but I suppose I should direct such requests at the predicate crate instead.

I actually have more wrappers around assert_cmd than around assert_fs: a lot of my tests involve running multiple programs inside a shared TempDir, killing them, chaining them, modifying env, providing stdin, capturing stdout/err, and looking at the resulting files when all is done. I think it's an API that would be worth upstreaming, but time is lacking at the moment.

@epage
Copy link
Contributor Author

epage commented Apr 1, 2019

@vincentdephily thanks for taking the time to look back and write up your thoughts!

which would be nice to a a convenience function for (TempDir.child_to_string() or something), but maybe that's too specific.

What reason are you reading the string for that isn't handled by predicates?

I also often test the file content with a regexp, but I suppose I should direct such requests at the predicate crate instead.

Yes, predicates already supports regex.

I actually have more wrappers around assert_cmd than around assert_fs: a lot of my tests involve running multiple programs inside a shared TempDir, killing them, chaining them, modifying env, providing stdin, capturing stdout/err, and looking at the resulting files when all is done. I think it's an API that would be worth upstreaming, but time is lacking at the moment.

Oh wow, sounds like some fun tests you've got there :). Even if you don't take the time to upstream, I'd be interested in you recording your workflow on assert-rs/assert_cmd#74 so we can see if its something that fits within assert_cmd and we can take on upstreaming it.

volks73 added a commit to volks73/cargo-wix that referenced this issue Sep 3, 2019
This required changing the implementation of creating test projects. The `.tmp`
prefix can be used in Windows but the cargo project name cannot. Thus, the
`--name <NAME>` option needed to be used for creating a test package because the
`TempDir` type from the assert_fs package does not support the tempfile TempDir
builder or changing the prefix in newer versions. See Issues [#46] and [#48]
from the `assert_fs` project for more information.

[#46]: assert-rs/assert_fs#46
[#48]: assert-rs/assert_fs#48
@epage
Copy link
Contributor Author

epage commented Nov 29, 2019

With #56's breaking change in, I don't think there is anything to hold us back from a 1.0. While the documentation improvements would be nice, we shouldn't wait for perfection.

@epage epage closed this as completed Nov 29, 2019
@epage epage unpinned this issue Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Uncertainty is involved
Projects
None yet
Development

No branches or pull requests

4 participants