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 <[email protected]> Change-Id: I3a7bb1f2e02b3e1545c24314a1b5200afeb23896 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1200274 Reviewed-by: Daniel Martí <[email protected]> Unity-Result: CUE porcuepine <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
cue-lang · Aug 30, 2024 · 9956f16 · 9956f16
1 parent 3deadce
commit 9956f16
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 79 deletions.
diff --git a/encoding/jsonschema/external_test.go b/encoding/jsonschema/external_test.go
@@ -15,12 +15,9 @@
 package jsonschema_test
 
 import (
-	"bytes"
-	stdjson "encoding/json"
 	"fmt"
 	"os"
 	"path"
-	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
@@ -37,10 +34,17 @@ import (
 	"cuelang.org/go/internal/cuetest"
 )
 
+// Pull in the external test data.
+// The commit below references the JSON schema test main branch as of Sun May 19 19:01:03 2024 +0300
+
+//go:generate go run vendor_external.go -- 9fc880bfb6d8ccd093bc82431f17d13681ffae8e
+
+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))
 
 	// Group the tests under a single subtest so that we can use
@@ -62,19 +66,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) {
@@ -109,21 +105,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(testName(test.Description), func(t *testing.T) {
@@ -138,38 +128,16 @@ func runExternalSchemaTests(t *testing.T, filename string, s *externaltest.Schem
 			qt.Assert(t, qt.IsNil(instValue.Err()))
 			err = instValue.Unify(schemaValue).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.Skipf("skipping due to known error: %v", test.Skip)
-					}
-					t.Fatalf("error: %v", errors.Details(err, nil))
-				} else if test.Skip != "" {
-					t.Fatalf("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.Fatalf("unexpectedly more correct behavior (test failure) on skipped test")
+					testFailed(t, &test.Skip, "unexpected success")
+				} else {
+					testSucceeded(t, &test.Skip)
 				}
 			}
 		})
@@ -182,6 +150,39 @@ func testName(s string) string {
 	return strings.ReplaceAll(s, "/", "__")
 }
 
+// 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.Skipf("skipping due to known error: %v", *skipField)
+	}
+	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,

diff --git a/encoding/jsonschema/internal/externaltest/tests.go b/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,

diff --git a/encoding/jsonschema/vendor-external b/encoding/jsonschema/vendor-external
diff --git a/encoding/jsonschema/vendor_external.go b/encoding/jsonschema/vendor_external.go
@@ -14,9 +14,14 @@
 
 //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"
@@ -26,11 +31,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"
+	testRepo = "https://github.com/json-schema-org/JSON-Schema-Test-Suite.git"
+	testDir  = "testdata/external"
 )
 
 func main() {
@@ -61,8 +68,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"))
@@ -81,21 +95,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