encoding/jsonschema: avoid test regressions

Ideally, we will "ratchet forward" the tests, fixing old tests
without breaking any tests that are working.

This change makes it so that even when CUE_UPDATE is set,
the encoding/jsonschema external tests will not update
a working test to a failing test unless CUE_ALLOW_REGRESSIONS
is also set.

We change the vendor script so that it will preserve test-skip
information even when the tests themselves are updated.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: I3a7bb1f2e02b3e1545c24314a1b5200afeb23896
Dispatch-Trailer: {"type":"trybot","CL":1200274,"patchset":1,"ref":"refs/changes/74/1200274/1","targetBranch":"master"}

encoding/jsonschema/external_test.go

@@ -1,12 +1,9 @@
 package jsonschema_test
 
 import (
-	"bytes"
-	stdjson "encoding/json"
 	"fmt"
 	"os"
 	"path"
-	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
@@ -23,10 +20,12 @@ import (
 	"cuelang.org/go/internal/cuetest"
 )
 
+const testDir = "testdata/external"
+
 // TestExternal runs the externally defined JSON Schema test suite,
 // as defined in https://github.com/json-schema-org/JSON-Schema-Test-Suite.
 func TestExternal(t *testing.T) {
-	tests, err := externaltest.ReadTestDir("testdata/external")
+	tests, err := externaltest.ReadTestDir(testDir)
 	qt.Assert(t, qt.IsNil(err))
 
 	t.Logf("read %d test files", len(tests))
@@ -49,19 +48,11 @@ func TestExternal(t *testing.T) {
 	if !cuetest.UpdateGoldenFiles {
 		return
 	}
-	for filename, schemas := range tests {
-		filename = filepath.Join("testdata/external", filename)
-		data, err := stdjson.MarshalIndent(schemas, "", "\t")
-		qt.Assert(t, qt.IsNil(err))
-		data = append(data, '\n')
-		oldData, err := os.ReadFile(filename)
-		qt.Assert(t, qt.IsNil(err))
-		if bytes.Equal(oldData, data) {
-			continue
-		}
-		err = os.WriteFile(filename, data, 0o666)
-		qt.Assert(t, qt.IsNil(err))
+	if t.Failed() {
+		t.Fatalf("not writing test data back because of test failures")
 	}
+	err = externaltest.WriteTestDir(testDir, tests)
+	qt.Assert(t, qt.IsNil(err))
 }
 
 func runExternalSchemaTests(t *testing.T, filename string, s *externaltest.Schema) {
@@ -96,21 +87,15 @@ func runExternalSchemaTests(t *testing.T, filename string, s *externaltest.Schem
 	}
 
 	if extractErr != nil {
-		if cuetest.UpdateGoldenFiles {
-			s.Skip = fmt.Sprintf("extract error: %v", extractErr)
-			for _, t := range s.Tests {
-				t.Skip = "could not compile schema"
-			}
-			return
-		}
-		if s.Skip != "" {
-			t.SkipNow()
+		for _, test := range s.Tests {
+			t.Run("", func(t *testing.T) {
+				testFailed(t, &test.Skip, "could not compile schema")
+			})
 		}
-		t.Fatalf("extract error: %v", extractErr)
-	}
-	if s.Skip != "" {
-		t.Errorf("unexpected test success on skipped test")
+		testFailed(t, &s.Skip, fmt.Sprintf("extract error: %v", extractErr))
+		return
 	}
+	testSucceeded(t, &s.Skip)
 
 	for _, test := range s.Tests {
 		t.Run("", func(t *testing.T) {
@@ -126,44 +111,55 @@ func runExternalSchemaTests(t *testing.T, filename string, s *externaltest.Schem
 			qt.Assert(t, qt.IsNil(instv.Err()))
 			err = instv.Unify(schemav).Err()
 			if test.Valid {
-				if cuetest.UpdateGoldenFiles {
-					if err == nil {
-						test.Skip = ""
-					} else {
-						test.Skip = errors.Details(err, nil)
-					}
-					return
-				}
 				if err != nil {
-					if test.Skip != "" {
-						t.SkipNow()
-					}
-					t.Fatalf("error: %v", errors.Details(err, nil))
-				} else if test.Skip != "" {
-					t.Error("unexpectedly more correct behavior (test success) on skipped test")
+					testFailed(t, &test.Skip, errors.Details(err, nil))
+				} else {
+					testSucceeded(t, &test.Skip)
 				}
 			} else {
-				if cuetest.UpdateGoldenFiles {
-					if err != nil {
-						test.Skip = ""
-					} else {
-						test.Skip = "unexpected success"
-					}
-					return
-				}
 				if err == nil {
-					if test.Skip != "" {
-						t.SkipNow()
-					}
-					t.Fatal("unexpected success")
-				} else if test.Skip != "" {
-					t.Error("unexpectedly more correct behavior (test failure) on skipped test")
+					testFailed(t, &test.Skip, "unexpected success")
+				} else {
+					testSucceeded(t, &test.Skip)
 				}
 			}
 		})
 	}
 }
 
+// testFailed marks the current test as failed with the
+// given error message, and updates the
+// skip field pointed to by skipField if necessary.
+func testFailed(t *testing.T, skipField *string, errStr string) {
+	if cuetest.UpdateGoldenFiles {
+		if *skipField == "" && !allowRegressions() {
+			t.Fatalf("test regression; was succeeding, now failing: %v", errStr)
+		}
+		*skipField = errStr
+		return
+	}
+	if *skipField != "" {
+		t.SkipNow()
+	}
+	t.Fatal(errStr)
+}
+
+// testFails marks the current test as succeeded and updates the
+// skip field pointed to by skipField if necessary.
+func testSucceeded(t *testing.T, skipField *string) {
+	if cuetest.UpdateGoldenFiles {
+		*skipField = ""
+		return
+	}
+	if *skipField != "" {
+		t.Fatalf("unexpectedly more correct behavior (test success) on skipped test")
+	}
+}
+
+func allowRegressions() bool {
+	return os.Getenv("CUE_ALLOW_REGRESSIONS") != ""
+}
+
 var extVersionToVersion = map[string]jsonschema.Version{
 	"draft3":       jsonschema.VersionUnknown,
 	"draft4":       jsonschema.VersionDraft4,

encoding/jsonschema/internal/externaltest/tests.go

@@ -2,8 +2,11 @@ package externaltest
 
 import (
 	"bytes"
+	"encoding/json"
 	stdjson "encoding/json"
+	"fmt"
 	"os"
+	"path/filepath"
 
 	"cuelang.org/go/cue"
 	"cuelang.org/go/cue/cuecontext"
@@ -27,7 +30,53 @@ type Test struct {
 	Skip        string             `json:"skip,omitempty"`
 }
 
+func ParseTestData(data []byte) ([]*Schema, error) {
+	var schemas []*Schema
+	if err := json.Unmarshal(data, &schemas); err != nil {
+		return nil, err
+	}
+	return schemas, nil
+}
+
+// WriteTestDir writes test data files as read by ReadTestDir
+// to the given directory. The keys of tests are filenames relative
+// to dir.
+func WriteTestDir(dir string, tests map[string][]*Schema) error {
+	for filename, schemas := range tests {
+		filename = filepath.Join(dir, filename)
+		data, err := stdjson.MarshalIndent(schemas, "", "\t")
+		if err != nil {
+			return err
+		}
+		if err != nil {
+			return err
+		}
+		data = append(data, '\n')
+		oldData, err := os.ReadFile(filename)
+		if err != nil {
+			return err
+		}
+		if bytes.Equal(oldData, data) {
+			continue
+		}
+		err = os.WriteFile(filename, data, 0o666)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+var ErrNotFound = fmt.Errorf("no external JSON schema tests found")
+
+// ReadTestDir reads all the external tests from the given directory.
 func ReadTestDir(dir string) (tests map[string][]*Schema, err error) {
+	if _, err := os.Stat(dir); err != nil {
+		if os.IsNotExist(err) {
+			return nil, ErrNotFound
+		}
+		return nil, err
+	}
 	os.Setenv("CUE_EXPERIMENT", "embed")
 	inst := load.Instances([]string{"."}, &load.Config{
 		Dir: dir,

encoding/jsonschema/vendor-external

@@ -12,6 +12,3 @@ go test
 
 # Then fetch the new tests.
 go run vendor_external.go -- $COMMIT
-
-# Then update the test results.
-CUE_UPDATE=1 go test

encoding/jsonschema/vendor_external.go

@@ -1,8 +1,13 @@
 //go:build ignore
 
+// This command copies external JSON Schema tests into the local
+// repository. It tries to maintain existing test-skip information
+// to avoid unintentional regressions.
+
 package main
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"io/fs"
@@ -12,11 +17,13 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+
+	"cuelang.org/go/encoding/jsonschema/internal/externaltest"
 )
 
 const (
 	testRepo = "git@github.com:json-schema-org/JSON-Schema-Test-Suite"
-	testDir  = "testdata/external/tests"
+	testDir  = "testdata/external"
 )
 
 func main() {
@@ -47,8 +54,15 @@ func doVendor(commit string) error {
 	if err := runCmd(tmpDir, "git", "checkout", "-q", commit); err != nil {
 		return err
 	}
+	logf("reading old test data")
+	oldTests, err := externaltest.ReadTestDir(testDir)
+	if err != nil && !errors.Is(err, externaltest.ErrNotFound) {
+		return err
+	}
 	logf("copying files to %s", testDir)
-	if err := os.RemoveAll(testDir); err != nil {
+
+	testSubdir := filepath.Join(testDir, "tests")
+	if err := os.RemoveAll(testSubdir); err != nil {
 		return err
 	}
 	fsys := os.DirFS(filepath.Join(tmpDir, "tests"))
@@ -67,21 +81,72 @@ func doVendor(commit string) error {
 		if !strings.HasSuffix(filename, ".json") {
 			return nil
 		}
-		if err := os.MkdirAll(filepath.Join(testDir, path.Dir(filename)), 0o777); err != nil {
+		if err := os.MkdirAll(filepath.Join(testSubdir, path.Dir(filename)), 0o777); err != nil {
 			return err
 		}
 		data, err := fs.ReadFile(fsys, filename)
 		if err != nil {
 			return err
 		}
-		if err := os.WriteFile(filepath.Join(testDir, filename), data, 0o666); err != nil {
+		if err := os.WriteFile(filepath.Join(testSubdir, filename), data, 0o666); err != nil {
 			return err
 		}
 		return nil
 	})
+
+	// Read the test data back that we've just written and attempt
+	// to populate skip data from the original test data.
+	// As indexes are not necessarily stable (new test cases
+	// might be inserted in the middle of an array, we try
+	// first to look up the skip info by JSON data, and then
+	// by test description.
+	byJSON := make(map[skipKey]string)
+	byDescription := make(map[skipKey]string)
+
+	for filename, schemas := range oldTests {
+		for _, schema := range schemas {
+			byJSON[skipKey{filename, string(schema.Schema), ""}] = schema.Skip
+			byDescription[skipKey{filename, schema.Description, ""}] = schema.Skip
+			for _, test := range schema.Tests {
+				byJSON[skipKey{filename, string(schema.Schema), string(test.Data)}] = test.Skip
+				byDescription[skipKey{filename, schema.Description, test.Description}] = schema.Skip
+			}
+		}
+	}
+
+	newTests, err := externaltest.ReadTestDir(testDir)
+	if err != nil {
+		return err
+	}
+
+	for filename, schemas := range newTests {
+		for _, schema := range schemas {
+			skip, ok := byJSON[skipKey{filename, string(schema.Schema), ""}]
+			if !ok {
+				skip, _ = byDescription[skipKey{filename, schema.Description, ""}]
+			}
+			schema.Skip = skip
+			for _, test := range schema.Tests {
+				skip, ok := byJSON[skipKey{filename, string(schema.Schema), string(test.Data)}]
+				if !ok {
+					skip, _ = byDescription[skipKey{filename, schema.Description, test.Description}]
+				}
+				test.Skip = skip
+			}
+		}
+	}
+	if err := externaltest.WriteTestDir(testDir, newTests); err != nil {
+		return err
+	}
 	return err
 }
 
+type skipKey struct {
+	filename string
+	schema   string
+	test     string
+}
+
 func runCmd(dir string, name string, args ...string) error {
 	c := exec.Command(name, args...)
 	c.Dir = dir