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

Send rspec approval failures to rspec for diffing #9

Merged
merged 2 commits into from
Dec 9, 2013

Conversation

jeremyruppel
Copy link
Contributor

Hey @kytrinyx, I thought it might be nice to have approval failures automatically show a diff in rspec. Let me know what you think!

@kytrinyx
Copy link
Contributor

I implemented this at first, and then removed it, because if even one of the failing specs is not the approval, then it's a pain to page through all the approval output (typically dozens to hundreds of lines) to find the error message.

@kytrinyx
Copy link
Contributor

@jeremyruppel Have you used the library much over the past 8 months? Do you still think that it would be worth having the diff in rspec?

I'd be interested in having the discussion again, if you're up for it :)

@bmarini
Copy link
Contributor

bmarini commented Dec 4, 2013

@kytrinyx I've been using it pretty heavily to test json output for an API (usually pretty small chunks, 10-20 lines of json), and having a diff would be really nice. Especially when a spec fails on your CI server but not locally, where there is not an easy way to see what the diff is.

@bmarini
Copy link
Contributor

bmarini commented Dec 4, 2013

Incidentally @jeremyruppel is my buddy and he is one who introduced me to the gem :)

@kytrinyx
Copy link
Contributor

kytrinyx commented Dec 4, 2013

Cool! So how about an option that could be passed to the approval to turn the diff off in certain cases? (I sometimes use this for characterization tests of massive, legacy HTML pages).

Approvals.verify(your_subject, :format => :html, :inspect => false) # or :diff, or :show_diff or something

@jeremyruppel
Copy link
Contributor Author

@kytrinyx @bmarini heya! Sorry for taking so long to get back to this. I definitely agree about making diffs optional. My only concern is that the diffs are dependent on RSpec and only available through the DSL in this implementation. It could make sense as an RSpec config, but that kind of implies the configuration for the whole test suite (which doesn't sound like it fits @kytrinyx's use case, amirite?). We could control it with RSpec metadata, but that feels way less intuitive to me as we're already passing our options to verify.

Hrm.

@kytrinyx
Copy link
Contributor

kytrinyx commented Dec 8, 2013

I was thinking of making it configurable on a test-by-test basis, but I'd be fine with having it be a global-for-rspec config as a first pass.

Want to take a stab at that in this pull request? Or would you rather just pull this in and then add the config option if it becomes too obnoxious?

@jeremyruppel
Copy link
Contributor Author

Okay! Diffs are now off by default and can be configured either for the whole suite or per-example using metadata.

@bmarini
Copy link
Contributor

bmarini commented Dec 9, 2013

I like it!

kytrinyx added a commit that referenced this pull request Dec 9, 2013
Send rspec approval failures to rspec for diffing
@kytrinyx kytrinyx merged commit deb4fe1 into approvals:master Dec 9, 2013
@kytrinyx
Copy link
Contributor

kytrinyx commented Dec 9, 2013

Thank you so much for the contribution, and I apologise for taking forever about it.

@jeremyruppel jeremyruppel deleted the failure-diffs branch December 10, 2013 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants