From d7d961388f77a1326eeb28abe5df9f42fb742fc0 Mon Sep 17 00:00:00 2001 From: Roger Peppe Date: Thu, 29 Aug 2024 17:53:17 +0100 Subject: [PATCH] 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 Change-Id: I3a7bb1f2e02b3e1545c24314a1b5200afeb23896 Dispatch-Trailer: {"type":"trybot","CL":1200274,"patchset":1,"ref":"refs/changes/74/1200274/1","targetBranch":"master"} --- encoding/jsonschema/external_test.go | 110 +++++++++--------- .../jsonschema/internal/externaltest/tests.go | 49 ++++++++ encoding/jsonschema/vendor-external | 3 - encoding/jsonschema/vendor_external.go | 73 +++++++++++- 4 files changed, 171 insertions(+), 64 deletions(-) diff --git a/encoding/jsonschema/external_test.go b/encoding/jsonschema/external_test.go index 4e60fe0b5..97129f219 100644 --- a/encoding/jsonschema/external_test.go +++ b/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, diff --git a/encoding/jsonschema/internal/externaltest/tests.go b/encoding/jsonschema/internal/externaltest/tests.go index 89badd730..cbff1cbaf 100644 --- 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 index 1ab5a620f..f529da89d 100755 --- a/encoding/jsonschema/vendor-external +++ b/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 diff --git a/encoding/jsonschema/vendor_external.go b/encoding/jsonschema/vendor_external.go index 5ebee7c17..933afdbe3 100644 --- a/encoding/jsonschema/vendor_external.go +++ b/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