Repository: vitessio/vitess
Pull request #11708 (2022-11-15 11:39:22)

Readable plan tests
## Description
Using a nicer diff when comparing JSONs. Makes the tests so much easier to read.

Here I changed `sharded` from true to false in a test

With the new diff tool:
```json
        {
            "Instructions": {
                "FieldQuery": "select `user`.col + 2 as a from `user` where 1 != 1",
                "Keyspace": {
                    "Name": "user",
                    "Sharded": false => true
                },
                "OperatorType": "Route",
                "Query": "select `user`.col + 2 as a from `user` having a = 42",
                "Table": "`user`",
                "Variant": "Scatter"
            },
            "Original": "select user.col + 2 as a from user having a = 42",
            "QueryType": "SELECT"
        }
```

Using the old tool:
```
        Diff:
          any(
        - 	json.RawMessage("{\n      \"QueryType\": \"SELECT\",\n      \"Original\": \"select user.col + 2 as a from user having a = 42\",\n      \"Instructions\": {\n        \"OperatorType\": \"Route\",\n        \"Variant\": \"Scatter\",\n        \"Keyspace\": {\n          \"Name\": \"user\",\n          \"Sharded\":"...),
        + 	string("{\n  \"QueryType\": \"SELECT\",\n  \"Original\": \"select user.col + 2 as a from user having a = 42\",\n  \"Instructions\": {\n    \"OperatorType\": \"Route\",\n    \"Variant\": \"Scatter\",\n    \"Keyspace\": {\n      \"Name\": \"user\",\n      \"Sharded\": true\n    },\n    \"FieldQuery\": \""...),
          )
```


## Related Issue(s)

<!-- List related issues and pull requests. If this PR fixes an issue, please add it using Fixes #????  -->

## Checklist

-   [x] "Backport to:" labels have been added if this change should be back-ported
-   [x] Tests were added or are not required
-   [x] Documentation was added or is not required

## Deployment Notes

<!-- Notes regarding deployment of the contained body of work. These should note any db migrations, etc. -->


vitess-bot[bot]: ### Review Checklist
            
Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

#### General
- [ ] Ensure that the Pull Request has a descriptive title.
- [ ] If this is a change that users need to know about, please apply the `release notes (needs details)` label so that merging is blocked unless the summary release notes document is included.

#### If a new flag is being introduced:
- [ ] Is it really necessary to add this flag?
- [ ] Flag names should be clear and intuitive (as far as possible)
- [ ] Help text should be descriptive.
- [ ] Flag names should use dashes (`-`) as word separators rather than underscores (`_`).

#### If a workflow is added or modified:
- [ ] Each item in `Jobs` should be named in order to mark it as `required`.
- [ ] If the workflow should be required, the maintainer team should be notified.

#### Bug fixes
- [ ] There should be at least one unit or end-to-end test.
- [ ] The Pull Request description should include a link to an issue that describes the bug.

#### Non-trivial changes
- [ ] There should be some code comments as to why things are implemented the way they are.

#### New/Existing features
- [ ] Should be documented, either by modifying the existing documentation or creating new documentation.
- [ ] New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

#### Backward compatibility            
- [ ] Protobuf changes should be wire-compatible.
- [ ] Changes to `_vt` tables and RPCs need to be backward compatible.
- [ ] `vtctl` command output order should be stable and `awk`-able.
- [ ] RPC changes should be compatible with vitess-operator
- [ ] If a flag is removed, then it should also be removed from VTop, if used there.


Patch:
diff --git a/go.mod b/go.mod
index 97122cfee8..aeafac47ac 100644
--- a/go.mod
+++ b/go.mod
@@ -117,6 +117,7 @@ require (
 
 require (
 	github.com/bndr/gotabulate v1.1.2
+	github.com/nsf/jsondiff v0.0.0-20210926074059-1e845ec5d249
 	github.com/openark/golib v0.0.0-20210531070646-355f37940af8
 	golang.org/x/exp v0.0.0-20221023144134-a1e5550cf13e
 )
diff --git a/go.sum b/go.sum
index a0ed58c44d..bcff557d4b 100644
--- a/go.sum
+++ b/go.sum
@@ -128,8 +128,6 @@ github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdn
 github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
 github.com/bndr/gotabulate v1.1.2 h1:yC9izuZEphojb9r+KYL4W9IJKO/ceIO8HDwxMA24U4c=
 github.com/bndr/gotabulate v1.1.2/go.mod h1:0+8yUgaPTtLRTjf49E8oju7ojpU11YmXyvq1LbPAb3U=
-github.com/buger/jsonparser v0.0.0-20200322175846-f7e751efca13 h1:+qUNY4VRkEH46bLUwxCyUU+iOGJMQBVibAaYzWiwWcg=
-github.com/buger/jsonparser v0.0.0-20200322175846-f7e751efca13/go.mod h1:tgcrVJ81GPSF0mz+0nu1Xaz0fazGPrmmJfJtxjbHhUQ=
 github.com/buger/jsonparser v1.1.1 h1:2PnMjfWD7wBILjqQbt530v576A/cAbQvEW9gGIpYMUs=
 github.com/buger/jsonparser v1.1.1/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
@@ -564,6 +562,8 @@ github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRW
 github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=
 github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw=
 github.com/ngdinhtoan/glide-cleanup v0.2.0/go.mod h1:UQzsmiDOb8YV3nOsCxK/c9zPpCZVNoHScRE3EO9pVMM=
+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/nxadm/tail v1.4.4 h1:DQuhQpB1tVlglWS2hLQ5OV6B5r8aGxSrPc5Qo6uTN78=
 github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A=
 github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo=


Test patch:
diff --git a/go/vt/vtgate/planbuilder/plan_test.go b/go/vt/vtgate/planbuilder/plan_test.go
index 70f89f88ec..11a5bf6a6b 100644
--- a/go/vt/vtgate/planbuilder/plan_test.go
+++ b/go/vt/vtgate/planbuilder/plan_test.go
@@ -29,6 +29,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/nsf/jsondiff"
 	"github.com/stretchr/testify/require"
 
 	vtgatepb "vitess.io/vitess/go/vt/proto/vtgate"
@@ -40,8 +41,6 @@ import (
 	"vitess.io/vitess/go/mysql/collations"
 	"vitess.io/vitess/go/vt/vtgate/semantics"
 
-	"github.com/google/go-cmp/cmp"
-
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/vterrors"
 
@@ -716,19 +715,9 @@ type (
 	}
 )
 
-func compacted(in string) string {
-	if in != "" && in[0] != '{' {
-		return in
-	}
-	dst := bytes.NewBuffer(nil)
-	err := json.Compact(dst, []byte(in))
-	if err != nil {
-		panic(err)
-	}
-	return dst.String()
-}
-
 func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, render bool) {
+	opts := jsondiff.DefaultConsoleOptions()
+
 	t.Run(filename, func(t *testing.T) {
 		var expected []planTest
 		var outFirstPlanner string
@@ -756,10 +745,9 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, r
 				}
 				out := getPlanOrErrorOutput(err, plan)
 
-				lft := compacted(out)
-				rgt := compacted(string(tcase.V3Plan))
-				if lft != rgt {
-					t.Errorf("V3 - %s\nDiff:\n%s\n[%s] \n[%s]", filename, cmp.Diff(tcase.V3Plan, out), tcase.V3Plan, out)
+				compare, s := jsondiff.Compare(tcase.V3Plan, []byte(out), &opts)
+				if compare != jsondiff.FullMatch {
+					t.Errorf("V3 - %s\nDiff:\n%s\n[%s] \n[%s]", filename, s, tcase.V3Plan, out)
 				}
 
 				outFirstPlanner = out
@@ -781,11 +769,9 @@ func testFile(t *testing.T, filename, tempDir string, vschema *vschemaWrapper, r
 			//       with this last expectation, it is an error if the Gen4 planner
 			//       produces the same plan as the V3 planner does
 			t.Run(fmt.Sprintf("Gen4: %s", testName), func(t *testing.T) {
-				x := string(tcase.Gen4Plan)
-				s := compacted(out)
-				s2 := compacted(x)
-				if s != s2 {
-					t.Errorf("Gen4 - %s\nDiff:\n%s\n[%s] \n[%s]", filename, cmp.Diff(s, s2), tcase.Gen4Plan, out)
+				compare, s := jsondiff.Compare(tcase.Gen4Plan, []byte(out), &opts)
+				if compare != jsondiff.FullMatch {
+					t.Errorf("Gen4 - %s\nDiff:\n%s\n[%s] \n[%s]", filename, s, tcase.Gen4Plan, out)
 				}
 
 				if outFirstPlanner == out {