-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
assert: add option to colorize diffs produced by assert.Equal #1480
base: master
Are you sure you want to change the base?
Conversation
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 MR contains 2 changes:
- switch the diff package
- use the colored diff of the new package.
I would prefer if you could post a separate MR that contains only the library change, and once merged, rebase this one.
@@ -6,19 +6,20 @@ import ( | |||
"encoding/json" | |||
"errors" | |||
"fmt" | |||
"github.com/sergi/go-diff/diffmatchpatch" |
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 import should not be in the block with stdlib imports.
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.
curious if we have explored go-cmp
package for diffs.
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.
@peymanmortazavi I haven't.
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.
@peymanmortazavi I considered using go-cmp
initally. Unfortunately go-cmp
cannot produce structured diffs so we can't customize them. This is why I decided to use diffmatchpatch here
Related: #1467. |
@@ -410,9 +411,9 @@ func Equal(t TestingT, expected, actual interface{}, msgAndArgs ...interface{}) | |||
return Fail(t, fmt.Sprintf("Invalid operation: %#v == %#v (%s)", | |||
expected, actual, err), msgAndArgs...) | |||
} | |||
|
|||
coloredOutput, _ := strconv.Atoi(os.Getenv("TESTIFY_COLORED_DIFF")) |
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 might be nice to store this value on the suite once after evaluation so we don't have to check on every Equal
call.
Additionally, how do you feel about using a function to recognize yes
|no and true|false
as well.
func prettyDiff(diffs []diffmatchpatch.Diff, useColoredOutput bool) string { | ||
var diff strings.Builder | ||
if useColoredOutput { | ||
diff.WriteString("\\033[31m--- Expected\\033[0m\n\\033[32m+++ Actual\\033[0m\n") |
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.
nit: it may be nice to use constants here for the colors.
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.
What's the right place for such constants? Should we keep them in the assert package or move it somewhere else? (e.g. to a colors
package)
Hello @mitioshi any plan to continue your work? Some suggestion: Cheers |
Aw, I didn't see the initial comments and thought this PR had been overlooked. |
This PR add an option to colorize diffs produced by assert.Equal
Changes
In this PR, the library used for diffing was replaced to diffmatchpatch which is actively maintained and enables us to access structured diffs.
Since testify is frequently used in CI/CD pipelines or piped to non-tty buffers, I made colorized diffs controlled by the TESTIFY_COLORED_DIFF environment variable. This way testify will not break any existing pipelines while letting the user control this behavior(e.g. via setting it in the go test template like this
)
Motivation
When running tests in IDE/text editor, it's difficult to analyze the diffs produced by assert.Equal. This could be improved by making diff colorized.
difflib is no longer maintained and cannot be extended to modify the diff structure, so it might be better to switch to another library: https://github.com/sergi/go-diff
The resulting diff looks like this after the suggested changes
The relevant issue: #1479