-
Notifications
You must be signed in to change notification settings - Fork 44
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
make json format approvals resilient to differences in pretty json fo… #85
make json format approvals resilient to differences in pretty json fo… #85
Conversation
0d776cb
to
4cc4c24
Compare
4cc4c24
to
ae1b0ad
Compare
@@ -105,6 +105,11 @@ def filter(data) | |||
Approvals.verify json, :format => :json, :namer => namer | |||
end | |||
|
|||
it "verifies json where the pretty generation is different but the content is the same" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add some tests how diffs show when you changed something, and the pretty generation is different of the approved and received file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as i saw the diff is generated by rspec by just diffing what is written in approved.json
and received.json
Not sure how i could test that, as that is simply read from those files by the ApprovalError
class. Since the contents of those files still is different because of the differences in the JSON gem, this will still show whitespace differences. But it also will show those when you check a changed approval into git that was generated on a different ruby.
I guess there is no sane workaround from that but using a pure ruby json generator. But i did not find any good one.
attr_accessor :approved_path, :received_path | ||
|
||
def approved | ||
@approved = JSON.parse(File.read(approved_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the instance variables?
22c622d
to
d724458
Compare
@kytrinyx there is some problem with nokogiri which fails randomly on travis:
Not sure why that happens randomly? is that a known problem? |
@Goltergaul Sorry for the slow reply. I've been away for the past 3 weeks, and am catching up on all my notifications today. I am not sure what the nokogiri failures are about. I wonder if Travis has a chat room or IRC channel to ask them about it. I'll give this a proper review in the next couple of days! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I just have the one question about the verify
method.
def verify | ||
return false unless approved == received | ||
true | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this couldn't be
def verify
- return false unless approved == received
- true
+ approved == received
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it definitely can... no idea why i wrote it that way. will change it
fails again because of nokogiri. Do we maybe need a version constraint for older rubies somewhere? |
We may! I'll look into the nokogiri thing. Meanwhile, this looks great, so I'm going to go ahead and merge it, nokokiri be damned. Thanks for taking the time to do this! I'll cut a release in the next few days so you don't have to work off of a SHA for your project. |
@kytrinyx cool thank you, could you pls ping me here when you have released the new version. |
Oh, shoot. This completely fell off my radar. Yes! I will get this done and ping you here. So sorry about that. |
Alright, v0.0.24 is released (I ended up having to skip v0.0.23 because I bumped the version before discovering that the actual release task would fail to run because of changes to the requirements about gem metadata). |
awesome thx <3 |
Pr for Issue #84
This is backwards compatible. The only thing that does not yet work perfectly is showing the diff if there is a real difference in the received json because here it will still show the diff to the raw approved file which might have different whitespaces. Those whitespace differences are then shown in the diff. (e.g. when you have
diff_on_approval_failure
turned on for rspec) But i think thats not such a big problem.What do you think?