-
Notifications
You must be signed in to change notification settings - Fork 928
GODRIVER-3868 Update extended bson cases. #2400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,14 +7,14 @@ | |||||
| package bson | ||||||
|
|
||||||
| import ( | ||||||
| "archive/tar" | ||||||
| "bytes" | ||||||
| "compress/gzip" | ||||||
| "encoding/json" | ||||||
| "errors" | ||||||
| "fmt" | ||||||
| "io" | ||||||
| "io/ioutil" | ||||||
| "os" | ||||||
| "path" | ||||||
| "sync" | ||||||
| "testing" | ||||||
| ) | ||||||
|
|
@@ -144,54 +144,74 @@ var nestedInstance = nestedtest1{ | |||||
| }, | ||||||
| } | ||||||
|
|
||||||
| const extendedBSONDir = "../testdata/extended_bson" | ||||||
| const extendedBSONTGZ = "../testdata/specifications/source/benchmarking/data/extended_bson.tgz" | ||||||
|
|
||||||
| var ( | ||||||
| extJSONFiles map[string]map[string]any | ||||||
| extJSONFilesMu sync.Mutex | ||||||
| ) | ||||||
|
|
||||||
| // readExtJSONFile reads the GZIP-compressed extended JSON document from the given filename in the | ||||||
| // "extended BSON" test data directory (../testdata/extended_bson) and returns it as a | ||||||
| // map[string]any. It panics on any errors. | ||||||
| func readExtJSONFile(filename string) map[string]any { | ||||||
| // readExtJSONFile reads the named JSON file from the extended_bson.tgz archive and returns it as a | ||||||
| // map[string]any. The first call decompresses the archive and caches all entries; subsequent calls | ||||||
| // only look up the cache. It calls b.Fatal on any errors. | ||||||
| func readExtJSONFile(b *testing.B, filename string) map[string]any { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [question] The original drivers ticket aimed to use strong types for these tests. Should we do the same? See this comment: https://github.com/mongodb/mongo-go-driver/pull/2400/changes#r3312898922
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The updates of strong-typed benchmarks are in "deep_bson.json.gz". They are verified in #2403. |
||||||
| b.Helper() | ||||||
| extJSONFilesMu.Lock() | ||||||
| defer extJSONFilesMu.Unlock() | ||||||
| if v, ok := extJSONFiles[filename]; ok { | ||||||
| return v | ||||||
| } | ||||||
| filePath := path.Join(extendedBSONDir, filename) | ||||||
| file, err := os.Open(filePath) | ||||||
| if err != nil { | ||||||
| panic(fmt.Sprintf("error opening file %q: %s", filePath, err)) | ||||||
| } | ||||||
| defer func() { | ||||||
| _ = file.Close() | ||||||
| }() | ||||||
|
|
||||||
| gz, err := gzip.NewReader(file) | ||||||
| if err != nil { | ||||||
| panic(fmt.Sprintf("error creating GZIP reader: %s", err)) | ||||||
| } | ||||||
| defer func() { | ||||||
| _ = gz.Close() | ||||||
| }() | ||||||
| if extJSONFiles == nil { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Suggest using sync.Once to help with readability: func readExtJSONFile(b *testing.B, filename string) map[string]any {
b.Helper()
extJSONFilesOnce.Do(func() {
// load into extJSONFiles
})
v, ok := extJSONFiles["extended_bson/"+filename]
if !ok {
b.Fatalf("file %q not found in %q", filename, extendedBSONTGZ)
return nil
}
return v
} |
||||||
| file, err := os.Open(extendedBSONTGZ) | ||||||
| if err != nil { | ||||||
| b.Fatalf("error opening %q: %s", extendedBSONTGZ, err) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return nil | ||||||
| } | ||||||
| defer func() { | ||||||
| _ = file.Close() | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocking] We should check this error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the closing error affects loading, and we don't need to log it either. |
||||||
| }() | ||||||
|
|
||||||
| gz, err := gzip.NewReader(file) | ||||||
| if err != nil { | ||||||
| b.Fatalf("error creating gzip reader: %s", err) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return nil | ||||||
| } | ||||||
| defer func() { | ||||||
| _ = gz.Close() | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocking] We should check this error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the closing error affects loading, and we don't need to log it either. |
||||||
| }() | ||||||
|
|
||||||
| data, err := ioutil.ReadAll(gz) | ||||||
| if err != nil { | ||||||
| panic(fmt.Sprintf("error reading GZIP contents of file: %s", err)) | ||||||
| } | ||||||
| extJSONFiles = make(map[string]map[string]any) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] It doesn't matter in this case but for hygiene suggest populating a local map and assigning this at the end of sync.Once, to avoid partially filling and then erroring. |
||||||
|
|
||||||
| var v map[string]any | ||||||
| err = UnmarshalExtJSON(data, false, &v) | ||||||
| if err != nil { | ||||||
| panic(fmt.Sprintf("error unmarshalling extended JSON: %s", err)) | ||||||
| tr := tar.NewReader(gz) | ||||||
| for { | ||||||
| hdr, err := tr.Next() | ||||||
| if errors.Is(err, io.EOF) { | ||||||
| break | ||||||
| } | ||||||
| if err != nil { | ||||||
| b.Fatalf("error reading tar: %s", err) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return nil | ||||||
|
|
||||||
| } | ||||||
| if hdr.Typeflag != tar.TypeReg { | ||||||
| continue | ||||||
| } | ||||||
| data, err := io.ReadAll(tr) | ||||||
| if err != nil { | ||||||
| b.Fatalf("error reading tar entry %q: %s", hdr.Name, err) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return nil | ||||||
| } | ||||||
| var v map[string]any | ||||||
| if err = UnmarshalExtJSON(data, false, &v); err != nil { | ||||||
| b.Fatalf("error unmarshalling %q: %s", hdr.Name, err) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return nil | ||||||
| } | ||||||
| extJSONFiles[hdr.Name] = v | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if extJSONFiles == nil { | ||||||
| extJSONFiles = make(map[string]map[string]any) | ||||||
| v, ok := extJSONFiles["extended_bson/"+filename] | ||||||
| if !ok { | ||||||
| b.Fatalf("file %q not found in %q", filename, extendedBSONTGZ) | ||||||
| return nil | ||||||
| } | ||||||
| extJSONFiles[filename] = v | ||||||
| return v | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -213,16 +233,16 @@ func BenchmarkMarshal(b *testing.B) { | |||||
| value: encodetestBsonD, | ||||||
| }, | ||||||
| { | ||||||
| desc: "deep_bson.json.gz", | ||||||
| value: readExtJSONFile("deep_bson.json.gz"), | ||||||
| desc: "deep_bson.json", | ||||||
| value: readExtJSONFile(b, "deep_bson.json"), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocking] We don't need to hardcode the files, just iterate over the results of decompressing the tarball. We should rename readExtJSONFile to loadExtendedBSON. Then we should iterate over the results: func TestLoadExtendedBSON(t *testing.T) {
loadExtendedBSON(t)
for filename, _ := range extJSONFiles {
t.Run(filename, func(t *testing.T) {
// Test / Benchmark
})
}
} |
||||||
| }, | ||||||
| { | ||||||
| desc: "flat_bson.json.gz", | ||||||
| value: readExtJSONFile("flat_bson.json.gz"), | ||||||
| desc: "flat_bson.json", | ||||||
| value: readExtJSONFile(b, "flat_bson.json"), | ||||||
| }, | ||||||
| { | ||||||
| desc: "full_bson.json.gz", | ||||||
| value: readExtJSONFile("full_bson.json.gz"), | ||||||
| desc: "full_bson.json", | ||||||
| value: readExtJSONFile(b, "full_bson.json"), | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -314,16 +334,16 @@ func BenchmarkUnmarshal(b *testing.B) { | |||||
| value: nestedInstance, | ||||||
| }, | ||||||
| { | ||||||
| name: "deep_bson.json.gz", | ||||||
| value: readExtJSONFile("deep_bson.json.gz"), | ||||||
| name: "deep_bson.json", | ||||||
| value: readExtJSONFile(b, "deep_bson.json"), | ||||||
| }, | ||||||
| { | ||||||
| name: "flat_bson.json.gz", | ||||||
| value: readExtJSONFile("flat_bson.json.gz"), | ||||||
| name: "flat_bson.json", | ||||||
| value: readExtJSONFile(b, "flat_bson.json"), | ||||||
| }, | ||||||
| { | ||||||
| name: "full_bson.json.gz", | ||||||
| value: readExtJSONFile("full_bson.json.gz"), | ||||||
| name: "full_bson.json", | ||||||
| value: readExtJSONFile(b, "full_bson.json"), | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change should only be applied for verification in #2403. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] Use testing.TB instead of *testing.B so that the loader is agnostic. In addition, I think we should make a test for the loader to ensure the structure is what we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use the hard-coded file names underneath to cross-verify the structure in the tarball is what we expect.