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

Add reporter based on the command line diff tool #29

Merged
merged 7 commits into from
Oct 8, 2021

Conversation

christoffer-gamrath
Copy link
Contributor

On behalf of my employer, Maersk, I am contributing this PR which consists of three changes:

  1. A reporter that prints the diff using the command line diff tool. The implementation is straight forwardly adapted from the other reporters. I believe adding this reporter will fix issue Logging diff on approval failure #27 in many cases given that Linux servers are likely to have some version of diff installed.
  2. Adding a call to t.Helper in the verify functions, so that when approval tests fail, the stacktrace will point to the call of the verify function instead of to some line within the library. This makes it easier for the user of the library to understand which assertion in their tests failed.
  3. Changing some pointers to interface values to plain interface values. It is unnecessary and unidiomatic to have pointers to interfaces in Go. Interfaces are already pointers internally. This should not affect any behavior.

Looking forward to your feedback.

Thanks,
Christoffer G. Thomsen
Senior Software Engineer
Maersk

So that when verification fails, the test runner will report the
line of code in your test that calls the verifier instead of a
line of code within the library.
It's unnecessary and unidiomatic. Interfaces can be satisfied by pointers or values.
The name DiffReporter was already taken by the composite reporter of all
the other reporters, so I came up with the name RealDiffReporter for
this one.
@hjwk
Copy link
Contributor

hjwk commented Oct 7, 2021

@emilybache, @objarni

@objarni
Copy link
Contributor

objarni commented Oct 7, 2021

Nice work @christoffer-thomsen !

I'm not a maintainer of this library, but as long as there are some automatic tests for the provided features I think this should just be merged 😊🎉

@emilybache emilybache merged commit ba414f7 into approvals:master Oct 8, 2021
@emilybache
Copy link
Contributor

great stuff, happy to have your input. Thankyou!

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

4 participants