[Feature][Generator Testing] More details in assert_file failure message

[Dana Scheider]

Hello team,

I'm writing to suggest a feature (which I'd easily be able to contribute). Currently, the assert_file method (in Rails::Generators::Testing::Assertions) has a failure message indicating that the file was not found. However, this method doesn't give any insight into whether the file was not created or was just named the wrong thing. This is a particular problem in the case of generators that use dynamically generated filenames.

Here's my use case: I'm working on testing a custom generator. My generator generates migrations for a second database. Like regular Rails migrations, this generator creates dynamic filenames that begin with a timestamp. When I'm testing the generator, it'd be useful to know whether the file has not been created or the timestamp just differs from that specified in the assertion.

It looks like this would be a trivial one-line change. Is this a PR that would be accepted?

Thanks!

Dana

[Xavier Noria]

Can you write some concrete examples? How would the calls look like? Which would be the assertion logic and error messages?

[Dana Scheider]

Sure, it wouldn't change the API at all. It'd just change this:

assert File.exist?(absolute), "Expected file #{relative.inspect} to exist, but does not"

To something more like this:

files = Dir[absolute]

assert File.exist?(absolute), 

  "Expected file #{relative.inspect} to exist, but does not. Existing files in this location are: #{files}"

Only the error message would really change.

On Tue, Sep 11, 2018 at 6:04 AM Xavier Noria <f...@hashref.com> wrote:

Can you write some concrete examples? How would the calls look like? Which would be the assertion logic and error messages?

--
You received this message because you are subscribed to a topic in the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rubyonrails-core/97lc7dB9OOg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rubyonrails-co...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
Visit this group at https://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.

[Dana Scheider]

I realized there are problems with that "after" code by the way - it was kind of back-of-the-envelope but hopefully gets the point across. It just changes the error message to list any existing files in the given directory.

[Rob Zolkos]

I think a more helpful exception message is handy for the developer.

I don't think we would need the list of existing files in the folder
(this could be huge). A simple "Expected file x was not found" would
be sufficient.

> You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.

[Dana Scheider]

That's the current behavior and it was not sufficient for me because it didn't tell me if the file hadn't been created or if it was just named the wrong thing. Let me think if maybe there's a better way to do this. Maybe we could generate a dynamic error message where if there's a small number (TBD) of files, it tells you what they are, and if there's a large number it just indicates that other files were found. It just seems useful to have that information when your filenames are created dynamically when the generator runs.

[Xavier Noria]

After thinking about it for a while, I am personally not in favor of adding this.

It is not a black & white argument, but a number of things that make me lean on that resolution.

I have grepped the Rails code base, which has +600 occurrences of this assertion and I believe listing the contents of the parent directory would be noisy and wouldn't help much in practice. Doesn't look like a good trade-off to me. If the file was not found, in most of the cases that's the deal, it wasn't generated. There is no point in listing siblings, running edit distances, or whatever, *by default*.

It would also introduce a lack of symmetry, to say it somehow, in the sense that then the implementation of `assert_file` would need to deal with the possibility that the parent directory doesn't exist either, or that the file exists but the test suite doesn't have enough permissions, for example.

I think the failure message is good as-is most of the time, and as such it is a good default.

If freezing time doesn't remove the "dynamic" bit of your use case, I believe it is a situation in which you need to manually print your listing in the test to debug the problem.