Allow use of composable matchers inside be_a_new#with

[twitnithegirl]

The be_a_new(model_class).with(attributes) did not allow the use of composable matchers inside the with method.

Fixes the third and final item in issue #1801.

This appears to be working as expected per my discussion with @myronmarston in the issue thread but I am, of course, open to feedback.

[myronmarston]

To work with all matchers, you need to delegate to the logic of each matcher. The way we do that for composable matchers is via the values_match?(expected, actual) method:

https://github.com/rspec/rspec-expectations/blob/0ad53b7b0a6e34b1d20f64b1c6588a26648cf6ed/lib/rspec/matchers/composable.rb#L51-L69

That said, the HaveAttributes matcher already does this (and more) and given that the attribute matching here should have the same semantics, I think it might be best if we just delegate to that matcher here:

def attributes_match?(actual)
  HaveAttributes.new(attributes).matches?(actual)
end

If we go that route, we'll probably want to delegate to HaveAttributes for part of the failure message, too.

All that said: I don't know much about rails these days and I'm not sure if the attributes method here needs to be used. @JonRowe or @samphippen -- can one of you advise?

[twitnithegirl]

Thanks! I'll dig in tomorrow and see where I get in that direction. I messed with it just now thinking it might just be that straight-forward...but then a bunch of tests blew up :/

[twitnithegirl]

One thing I've noticed upon further inspection is that while have_attributes does appear to use nested matchers the way you and the docs have said, there doesn't seem to actually be any testing around that in the have_attributes_spec. There are a couple a_string_includings in there but those are just in "fail_with." It might be good to add some test coverage around that. Thoughts?

[myronmarston]

I believe it is covered by tests here:

https://github.com/rspec/rspec-expectations/blob/0ad53b7b0a6e34b1d20f64b1c6588a26648cf6ed/spec/rspec/matchers/built_in/have_attributes_spec.rb#L83-L99

The test coverage there is pretty light because all the heavy lifting is done by other logic the matcher delegates to.

[twitnithegirl]

Eek, how did I miss that? Thanks, ugh.

[myronmarston]

It's a big spec file, and easy to miss!

[twitnithegirl]

I took a second stab using values_match? instead eql?. I might try to further refactor this later. I tried to directly use HaveAttributes and that's something else I might circle back to but I think this implementation does All The Things™

[twitnithegirl]

Hey @myronmarston do you mind checking out my latest commit when you get a sec? I messed around with HaveAttributes a bit but that didn't really seem like the cleanest way to go. Current commit seems to handle everything one might want to do.

[myronmarston]

Your changes look good, @britnia! My one suggestion is to make sure the description and failure_message are good. We don't just want passed matchers to be inspected in description and failure message strings; instead we want the description of passed matchers to be used you can see how have_attributes does it in description and formatted_values.

In the specs, it would be good to reduce (or eliminate) the use of regexes for matching the failure message. Once it no longer uses the Object#inspect output of the passed formatters it should be more deterministic and possible to match directly.

Does that make sense?

[twitnithegirl]

Yeah, totally. Thanks for the feedback.

[twitnithegirl]

@myronmarston I made the failure messages more accurate and caught my branch up to master. I also removed my use of regex matching however there is still quite a bit of it in the file. I am happy to edit the tests to not have them but that seems like a separate task from this issue.

[twitnithegirl]

I had some super weird merge/rebase issues that I just fixed so the git log should look a bit cleaner. Apparently I have now written these commits with....myself? Gotta keep it weird somehow I guess.

[myronmarston]

Well done, @britnia!