Skip to content

internal/testrunner/runners/pipeline: replace jsondiff output with difflib #872

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 28, 2022
Merged

internal/testrunner/runners/pipeline: replace jsondiff output with difflib #872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ require (
github.com/fatih/color v1.13.0
github.com/go-git/go-billy/v5 v5.3.1
github.com/go-git/go-git/v5 v5.4.2
github.com/google/go-cmp v0.5.7
github.com/google/go-github/v32 v32.1.0
github.com/google/go-querystring v1.1.0
github.com/jedib0t/go-pretty v4.3.0+incompatible
github.com/magefile/mage v1.13.0
github.com/mholt/archiver/v3 v3.5.1
github.com/nsf/jsondiff v0.0.0-20210926074059-1e845ec5d249
github.com/olekukonko/tablewriter v0.0.5
github.com/pkg/errors v0.9.1
github.com/pmezard/go-difflib v1.0.0
github.com/spf13/cobra v1.5.0
github.com/stretchr/testify v1.7.5
golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8
Expand Down Expand Up @@ -76,7 +77,6 @@ require (
github.com/golang/snappy v0.0.3 // indirect
github.com/google/btree v1.0.1 // indirect
github.com/google/gnostic v0.5.7-v3refs // indirect
github.com/google/go-cmp v0.5.7 // indirect
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
github.com/google/uuid v1.2.0 // indirect
Expand Down Expand Up @@ -112,7 +112,6 @@ require (
github.com/oklog/ulid v1.3.1 // indirect
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
github.com/pierrec/lz4/v4 v4.1.2 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/russross/blackfriday v1.5.2 // indirect
github.com/sergi/go-diff v1.1.0 // indirect
github.com/sirupsen/logrus v1.8.1 // indirect
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,8 +913,6 @@ github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+
github.com/ncw/swift v1.0.47/go.mod h1:23YIA4yWVnGwv2dQlN4bB7egfYX6YLn0Yo/S6zZO/ZM=
github.com/networkplumbing/go-nft v0.2.0/go.mod h1:HnnM+tYvlGAsMU7yoYwXEVLLiDW9gdMmb5HoGcwpuQs=
github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno=
github.com/nsf/jsondiff v0.0.0-20210926074059-1e845ec5d249 h1:NHrXEjTNQY7P0Zfx1aMrNhpgxHmow66XQtm0aQLY0AE=
github.com/nsf/jsondiff v0.0.0-20210926074059-1e845ec5d249/go.mod h1:mpRZBD8SJ55OIICQ3iWH0Yz3cjzA61JdqMLoWXeB2+8=
github.com/nwaples/rardecode v1.1.0 h1:vSxaY8vQhOcVr4mm5e8XllHWTiM4JF507A0Katqw7MQ=
github.com/nwaples/rardecode v1.1.0/go.mod h1:5DzqNKiOdpKKBH87u8VlvAnPZMXcGRhxWkRpHbbfGS0=
github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A=
Expand Down
50 changes: 32 additions & 18 deletions internal/testrunner/runners/pipeline/test_result.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ import (
"os"
"path/filepath"

"github.com/nsf/jsondiff"
"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"github.com/pmezard/go-difflib/difflib"

"github.com/elastic/elastic-package/internal/common"
"github.com/elastic/elastic-package/internal/testrunner"
Expand Down Expand Up @@ -103,24 +104,37 @@ func compareJsonNumbers(a, b json.Number) bool {
}

func diffJson(want, got []byte) (string, error) {
opts := jsondiff.DefaultConsoleOptions()

// Remove colored output.
opts.Added = jsondiff.Tag{Begin: "+ "}
opts.Removed = jsondiff.Tag{Begin: "- "}
opts.Changed = jsondiff.Tag{}
opts.Skipped = jsondiff.Tag{}

// Configure diff.
opts.SkipMatches = true
opts.CompareNumbers = compareJsonNumbers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compareJsonNumbers and its test can be removed if we stop using jsondiff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally thinking that, but the logic behind using it made a lot of sense, to I used it as the Comparer for cmp.Equal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you are right, I didn't see that it was still used there 👍

var gotVal, wantVal interface{}
err := jsonUnmarshalUsingNumber(want, &wantVal)
if err != nil {
return "", fmt.Errorf("invalid want value: %w", err)
}
err = jsonUnmarshalUsingNumber(got, &gotVal)
if err != nil {
return "", fmt.Errorf("invalid got value: %w", err)
}
if cmp.Equal(gotVal, wantVal, cmp.Comparer(compareJsonNumbers)) {
return "", nil
}

result, diff := jsondiff.Compare(want, got, &opts)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep this comparison method even if later we change the diff that we print, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right.

switch result {
case jsondiff.FirstArgIsInvalidJson, jsondiff.SecondArgIsInvalidJson, jsondiff.BothArgsAreInvalidJson:
return "", errors.New("invalid json")
got, err = marshalNormalizedJSON(gotVal)
if err != nil {
return "", err
}
return diff, nil
want, err = marshalNormalizedJSON(wantVal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marshalNormalizedJSON marshals and unmarshals using jsonUnmarshalUsingNumber, something that has been already done here. We could do the final marshal directly here instead of repeating the operations.
Not so important though as this code is only used if the documents are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was being conservative to ensure that the values were being treated the same way and to ensure that the reader was clear that this was happening.

if err != nil {
return "", err
}

var buf bytes.Buffer
err = difflib.WriteUnifiedDiff(&buf, difflib.UnifiedDiff{
A: difflib.SplitLines(string(want)),
B: difflib.SplitLines(string(got)),
FromFile: "want",
ToFile: "got",
Context: 3,
})
return buf.String(), err
}

func readExpectedTestResult(testCasePath string, config *testConfig) (*testResult, error) {
Expand Down Expand Up @@ -228,7 +242,7 @@ func marshalTestResultDefinition(result *testResult) ([]byte, error) {
// marshalNormalizedJSON marshals test results ensuring that field
// order remains consistent independent of field order returned by
// ES to minimize diff noise during changes.
func marshalNormalizedJSON(v testResultDefinition) ([]byte, error) {
func marshalNormalizedJSON(v interface{}) ([]byte, error) {
msg, err := json.Marshal(v)
if err != nil {
return msg, err
Expand Down