Merge pull request #27 from n3wscott/ce-diff

[diff] better findby, split on comma, add find tracker

pkg/commands/diff.go

@@ -7,6 +7,7 @@ package commands
 
 import (
 	"os"
+	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -24,6 +25,17 @@ func addDiff(topLevel *cobra.Command) {
   cloudevents diff ./want.yaml ./got.yaml
 `,
 		Args: cobra.ExactArgs(2),
+		PreRun: func(cmd *cobra.Command, args []string) {
+			var findBy []string
+			for _, fb := range do.FindBy {
+				if strings.Contains(fb, ",") {
+					findBy = append(findBy, strings.Split(fb, ",")...)
+				} else {
+					findBy = append(findBy, fb)
+				}
+			}
+			do.FindBy = findBy
+		},
 		Run: func(cmd *cobra.Command, args []string) {
 			r := diff.Diff{
 				Out:             cmd.OutOrStdout(),

pkg/diff/diff.go

@@ -8,13 +8,14 @@ package diff
 import (
 	"errors"
 	"fmt"
+	cmpdiff "github.com/kylelemons/godebug/diff"
 	"io"
+	"math"
 	"strings"
 
 	"github.com/cloudevents/conformance/pkg/event"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	cmpdiff "github.com/kylelemons/godebug/diff"
 )
 
 // Diff compares two CloudEvents yaml files (or directories) for differences,
@@ -30,109 +31,150 @@ type Diff struct {
 	FileB string
 }
 
-func (i *Diff) find(like event.Event, all []event.Event) (string, *event.Event) {
+var ignoreOpts = cmpopts.IgnoreFields(event.Event{}, "Mode", "TransportExtensions")
+
+func newTracker(findBy []string, all []event.Event) *tracker {
+	return &tracker{
+		findBy: findBy,
+		all:    all,
+		used:   make(map[int]bool, len(all)),
+	}
+}
+
+type tracker struct {
+	findBy []string
+	all    []event.Event
+	used   map[int]bool
+}
+
+type trackerFn func(string, event.Event)
+
+func (t *tracker) findBest(like event.Event, fn trackerFn) (string, error) {
+	// Make the label.
 	label := ""
-	for _, b := range i.FindBy {
+	for _, b := range t.findBy {
 		if len(label) == 0 {
 			label = fmt.Sprintf("%s[%s]", b, like.Get(b))
 		} else {
 			label = fmt.Sprintf("%s %s[%s]", label, b, like.Get(b))
 		}
 	}
 
-	// Look for the matching ID, Source and Subject.
-	for _, a := range all {
+	best := -1
+	bestDiff := math.MaxInt
+
+	// Look for like.
+	for i, b := range t.all {
+		if t.used[i] {
+			continue
+		}
+
+		// Test for a match.
 		match := true
-		for _, key := range i.FindBy {
-			if a.Get(key) != like.Get(key) {
+		for _, key := range t.findBy {
+			if like.Get(key) != b.Get(key) {
 				match = false
 				break
 			}
 		}
-		if match {
-			return label, &a
+		if !match {
+			continue
+		}
+		// Simple compute the "delta" between line and b, just the length of the diff.
+		diff := len(cmp.Diff(like, b, ignoreOpts))
+		if diff < bestDiff {
+			best = i
+			bestDiff = diff
 		}
 	}
-	return label, nil
+	if best >= 0 {
+		fn(label, t.all[best])
+		t.used[best] = true
+		return label, nil
+	}
+	return label, io.EOF
 }
 
 func (i *Diff) Do() error {
-	ignoreOpts := cmpopts.IgnoreFields(event.Event{}, "Mode", "TransportExtensions")
-
+	// Read and parse FileA
 	eventsA, err := event.FromYaml(i.FileA, true)
 	if err != nil {
 		return err
 	}
 
+	// Read and parse FileB
 	eventsB, err := event.FromYaml(i.FileB, true)
 	if err != nil {
 		return err
 	}
 
 	var diffs []string
 
-	for _, a := range eventsA {
-		label, b := i.find(a, eventsB)
-
-		if b == nil {
-			_, _ = fmt.Fprintf(i.Out, "missing: %s\n", label)
-			diffs = append(diffs, fmt.Sprintf("missing: %s\n", label))
-			continue
-		}
+	// Tracker helps manage selecting events based on findBy and the current event to compare.
+	t := newTracker(i.FindBy, eventsB)
 
-		if diff := cmp.Diff(a, b, ignoreOpts); diff != "" {
-			// Clear out Mode and Transport Extensions.
-			a.Mode = ""
-			b.Mode = ""
-			a.TransportExtensions = nil
-			b.TransportExtensions = nil
+	for _, a := range eventsA {
 
-			ab, err := event.ToYaml(a)
-			if err != nil {
-				return err
-			}
-			bb, err := event.ToYaml(*b)
-			if err != nil {
-				return err
-			}
+		if label, err := t.findBest(a, func(label string, b event.Event) {
+			if diff := cmp.Diff(a, b, ignoreOpts); diff != "" {
+				// Clear out Mode and Transport Extensions.
+				a.Mode = ""
+				b.Mode = ""
+				a.TransportExtensions = nil
+				b.TransportExtensions = nil
+
+				ab, err := event.ToYaml(a)
+				if err != nil {
+					return
+				}
+				bb, err := event.ToYaml(b)
+				if err != nil {
+					return
+				}
 
-			ignore := true
-			sb := &strings.Builder{}
-			if len(diffs) >= 1 {
-				sb.WriteString("---\n")
-			}
-			sb.WriteString(fmt.Sprintf("%s diffs (-a, +b):\n", label))
-
-			chunks := cmpdiff.DiffChunks(strings.Split(string(ab), "\n"), strings.Split(string(bb), "\n"))
-			changed := map[string]bool{}
-			for _, c := range chunks {
-				if len(c.Added) != 0 {
-					for _, a := range c.Added {
-						if _, found := changed[strings.Split(a, ":")[0]]; found || !i.IgnoreAdditions {
-							ignore = false
-							sb.WriteString(fmt.Sprintf("+ %s\n", a))
+				ignore := true
+				sb := &strings.Builder{}
+				if len(diffs) >= 1 {
+					sb.WriteString("---\n")
+				}
+				sb.WriteString(fmt.Sprintf("%s diffs (-a, +b):\n", label))
+
+				chunks := cmpdiff.DiffChunks(strings.Split(string(ab), "\n"), strings.Split(string(bb), "\n"))
+				changed := map[string]bool{}
+				for _, c := range chunks {
+					if len(c.Added) != 0 {
+						for _, a := range c.Added {
+							if _, found := changed[strings.Split(a, ":")[0]]; found || !i.IgnoreAdditions {
+								ignore = false
+								sb.WriteString(fmt.Sprintf("+ %s\n", a))
+							}
 						}
 					}
-				}
-				if len(c.Deleted) != 0 {
-					ignore = false
-					for _, d := range c.Deleted {
-						sb.WriteString(fmt.Sprintf("- %s\n", d))
-						changed[strings.Split(d, ":")[0]] = true
+					if len(c.Deleted) != 0 {
+						ignore = false
+						for _, d := range c.Deleted {
+							sb.WriteString(fmt.Sprintf("- %s\n", d))
+							changed[strings.Split(d, ":")[0]] = true
+						}
 					}
 				}
-			}
 
-			if i.FullDiff {
-				_, _ = fmt.Fprintf(i.Out, "%s\n", cmpdiff.Diff(string(ab), string(bb)))
-			} else if !ignore {
-				_, _ = fmt.Fprint(i.Out, sb.String())
-				if len(diffs) > 0 {
-					diffs = append(diffs, "---")
+				if i.FullDiff {
+					_, _ = fmt.Fprintf(i.Out, "%s\n", cmpdiff.Diff(string(ab), string(bb)))
+				} else if !ignore {
+					_, _ = fmt.Fprint(i.Out, sb.String())
+					if len(diffs) > 0 {
+						diffs = append(diffs, "---")
+					}
+					diffs = append(diffs, cmpdiff.Diff(string(ab), string(bb)))
 				}
-				diffs = append(diffs, cmpdiff.Diff(string(ab), string(bb)))
 			}
+		}); err != nil {
+			_, _ = fmt.Fprintf(i.Out, "missing: %s\n", label)
+			diffs = append(diffs, fmt.Sprintf("missing: %s\n", label))
+			continue
 		}
+
 	}
 
 	if len(diffs) > 0 {

pkg/diff/diff_test.go

@@ -64,6 +64,20 @@ type[com.example.someevent.c] diffs (-a, +b):
 -   source: /mycontext/subcontext
 +   source: /mycontext/subcontext2
 `,
+	}, {
+		name:            "sample7: two similar events, match second",
+		a:               "./testdata/sample7_a.yaml",
+		b:               "./testdata/sample7_b.yaml",
+		findBy:          []string{"type", "subject"},
+		ignoreAdditions: true,
+		wantErr:         false,
+	}, {
+		name:            "sample7: two similar events, two matches out of order, search just by type",
+		a:               "./testdata/sample7_a.yaml",
+		b:               "./testdata/sample7_b.yaml",
+		findBy:          []string{"type"},
+		ignoreAdditions: true,
+		wantErr:         false,
 	}}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/diff/testdata/sample7_a.yaml

@@ -0,0 +1,3 @@
+ContextAttributes:
+  type: com.example.someevent
+  subject: second-subject

pkg/diff/testdata/sample7_b.yaml

@@ -0,0 +1,33 @@
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:14Z
+  id: 4321-4321-4321-a
+  source: /mycontext/subcontext
+  subject: first-subject
+  Extensions:
+    comexampleextension1 : "value"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}
+---
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:24Z
+  id: 4321-4321-4321-b
+  source: /mycontext/subcontext
+  subject: second-subject
+  Extensions:
+    comexampleextension1 : "value"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}

pkg/diff/testdata/sample8_a.yaml

@@ -0,0 +1,7 @@
+ContextAttributes:
+  type: com.example.someevent
+  subject: second-subject
+---
+ContextAttributes:
+  type: com.example.someevent
+  subject: first-subject

pkg/diff/testdata/sample8_b.yaml

@@ -0,0 +1,65 @@
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:14Z
+  id: 4321-4321-4321-a
+  source: /mycontext/subcontext
+  Extensions:
+    comexampleextension1 : "this one has no subject"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}
+---
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:14Z
+  id: 4321-4321-4321-b
+  source: /mycontext/subcontext
+  subject: first-subject
+  Extensions:
+    comexampleextension1 : "value"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}
+---
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:24Z
+  id: 4321-4321-4321-c
+  subject: second-subject
+  Extensions:
+    comexampleextension1 : "this one has no subject"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}
+---
+Mode: structured
+ContextAttributes:
+  specversion: 1.0
+  type: com.example.someevent
+  time: 2018-04-05T03:56:24Z
+  id: 4321-4321-4321-d
+  source: /mycontext/subcontext
+  subject: second-subject
+  Extensions:
+    comexampleextension1 : "value"
+    comexampleextension2 : |
+      {"othervalue": 5}
+TransportExtensions:
+  user-agent: "foo"
+Data: |
+  {"world":"hello"}