Testing/round trip go

.github/workflows/bench_pr.yml

@@ -6,10 +6,10 @@ jobs:
     runs-on: ubuntu-latest
     steps:
 
-    - name: Set up Go 1.15
+    - name: Set up Go 1.16
       uses: actions/setup-go@v2
       with:
-        go-version: 1.15
+        go-version: 1.16
       id: go
 
     - name: Check out code 

.github/workflows/bench_push.yml

@@ -5,10 +5,10 @@ jobs:
     name: Benchmark (push)
     runs-on: ubuntu-latest
     steps:
-    - name: Set up Go 1.15
+    - name: Set up Go 1.16
       uses: actions/setup-go@v2
       with:
-        go-version: 1.15
+        go-version: 1.16
       id: go
 
     - name: Check out code 

.github/workflows/go.yml

@@ -7,10 +7,10 @@ jobs:
     runs-on: ubuntu-latest
     steps:
 
-    - name: Set up Go 1.15
+    - name: Set up Go 1.16
       uses: actions/setup-go@v2
       with:
-        go-version: 1.15
+        go-version: 1.16
       id: go
 
     - name: Check out code 

go.mod

@@ -1,6 +1,6 @@
 module github.com/suyashkumar/dicom
 
-go 1.13
+go 1.16
 
 require (
 	github.com/golang/mock v1.4.4

go.sum

@@ -2,12 +2,17 @@ github.com/golang/mock v1.4.4 h1:l75CXGRSwbaYNpl/Z2X1XIIAMSCquvXgpVZDhwEIJsc=
 github.com/golang/mock v1.4.4/go.mod h1:l3mdAwkq5BuhzHwde/uurv3sEJeZMXNpwsxVWU71h+4=
 github.com/google/go-cmp v0.5.2 h1:X2ev0eStA3AbceY54o37/0PQ/UWqKEiiO2dKL5OPaFM=
 github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
+golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 h1:VklqNMn3ovrHsnt90PveolxSbWFaJdECFbxSq0Mqo2M=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/net v0.0.0-20190311183353-d8887717615a h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628=
 golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
+golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
+golang.org/x/tools v0.0.0-20190425150028-36563e24a262 h1:qsl9y/CJx34tuA7QCPNp86JNJe4spst6Ff8MjvPUdPg=
 golang.org/x/tools v0.0.0-20190425150028-36563e24a262/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

parse_test.go

@@ -1,82 +1,50 @@
 package dicom_test
 
 import (
-	"bytes"
 	"encoding/json"
 	"fmt"
+	"github.com/suyashkumar/dicom/pkg/frame"
+	"github.com/suyashkumar/dicom/pkg/tag"
 	"image/jpeg"
-	"io/ioutil"
 	"os"
-	"strings"
 	"testing"
 
-	"github.com/suyashkumar/dicom/pkg/tag"
-
-	"github.com/suyashkumar/dicom/pkg/frame"
-
 	"github.com/suyashkumar/dicom"
+	"github.com/suyashkumar/dicom/pkg/dcmtest"
 )
 
-// TestParse is an end-to-end sanity check over DICOMs in testdata/. Currently it only checks that no error is returned
-// when parsing the files.
+// TestParse is an end-to-end sanity check over DICOMs in /pkg/dcmtest/data/. Currently
+// it only checks that no error is returned when parsing the files.
 func TestParse(t *testing.T) {
-	files, err := ioutil.ReadDir("./testdata")
-	if err != nil {
-		t.Fatalf("unable to read testdata/: %v", err)
-	}
-	for _, f := range files {
-		if !f.IsDir() && strings.HasSuffix(f.Name(), ".dcm") {
-			t.Run(f.Name(), func(t *testing.T) {
-				dcm, err := os.Open("./testdata/" + f.Name())
-				if err != nil {
-					t.Errorf("Unable to open %s. Error: %v", f.Name(), err)
-				}
-				defer dcm.Close()
-				info, err := dcm.Stat()
-				if err != nil {
-					t.Errorf("Unable to stat %s. Error: %v", f.Name(), err)
-				}
-				_, err = dicom.Parse(dcm, info.Size(), nil)
-				if err != nil {
-					t.Errorf("dicom.Parse(%s) unexpected error: %v", f.Name(), err)
-				}
-			})
+	// This will walk all of our test data and try parsing the Dataset, so we can simply
+	// report if we get passed an error.
+	dcmtest.WalkIncludedFS(t, func(t *testing.T, tc dcmtest.FSTestCase, setupErr error) {
+		if setupErr != nil {
+			t.Fatalf("setup error: %v", setupErr)
 		}
-	}
+	})
 }
 
-// BenchmarkParse runs sanity benchmarks over the sample files in testdata.
+// BenchmarkParse runs sanity benchmarks over the sample files in /pkg/dcmtest/data/.
 func BenchmarkParse(b *testing.B) {
-	files, err := ioutil.ReadDir("./testdata")
-	if err != nil {
-		b.Fatalf("unable to read testdata/: %v", err)
-	}
-	for _, f := range files {
-		if !f.IsDir() && strings.HasSuffix(f.Name(), ".dcm") {
-			b.Run(f.Name(), func(b *testing.B) {
-				dcm, err := os.Open("./testdata/" + f.Name())
-				if err != nil {
-					b.Errorf("Unable to open %s. Error: %v", f.Name(), err)
-				}
-				defer dcm.Close()
-
-				data, err := ioutil.ReadAll(dcm)
-				if err != nil {
-					b.Errorf("Unable to read file into memory for benchmark: %v", err)
-				}
+	// This will walk all of our test data and run a sub-test for each one.
+	dcmtest.BenchIncludedFS(b, func(b *testing.B, tc dcmtest.FSTestCase, setupErr error) {
+		// The test helper will have already parsed the file once, so we will use that
+		// as validation that the dataset is good.
+		if setupErr != nil {
+			b.Fatalf("setup error: %v", setupErr)
+		}
 
-				b.ResetTimer()
-				for i := 0; i < b.N; i++ {
-					_, _ = dicom.Parse(bytes.NewBuffer(data), int64(len(data)), nil)
-				}
-			})
+		b.ResetTimer()
+		for i := 0; i < b.N; i++ {
+			_, _ = dicom.Parse(tc.DCMFile, tc.DCMStat.Size(), nil)
 		}
-	}
+	})
 }
 
 func Example_readFile() {
 	// See also: dicom.Parse, which uses a more generic io.Reader API.
-	dataset, _ := dicom.ParseFile("testdata/1.dcm", nil)
+	dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil)
 
 	// Dataset will nicely print the DICOM dataset data out of the box.
 	fmt.Println(dataset)
@@ -99,12 +67,12 @@ func Example_streamingFrames() {
 		}
 	}()
 
-	dataset, _ := dicom.ParseFile("testdata/1.dcm", frameChan)
+	dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", frameChan)
 	fmt.Println(dataset) // The full dataset
 }
 
 func Example_getImageFrames() {
-	dataset, _ := dicom.ParseFile("testdata/1.dcm", nil)
+	dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil)
 	pixelDataElement, _ := dataset.FindElementByTag(tag.PixelData)
 	pixelDataInfo := dicom.MustGetPixelDataInfo(pixelDataElement.Value)
 	for i, fr := range pixelDataInfo.Frames {

pkg/dcmtest/data.go

@@ -0,0 +1,18 @@
+package dcmtest
+
+import (
+	"embed"
+)
+
+// IncludedFS uses the embed package to read all of the test files in /pkg/dcmtest/data
+// into an embed.FS for testing. This allows us to keep the files in memory without
+// reading them from disk each time they are needed, or worrying about relative import
+// paths.
+//
+// WARNING
+//
+// This var should only be used for testing purposes. It contains a number of full DICOM
+// files that could significantly bloat a binary's size if included in production code.
+//
+//go:embed **/*.dcm
+var IncludedFS embed.FS

pkg/dcmtest/doc.go

@@ -0,0 +1,9 @@
+// Package dcmtest contains helper methods and functions for testing against dicom
+// files.
+//
+// WARNING
+//
+// This module should only be used by test files or test helper packages to be run by
+// `gotest`, as it included an embedded dataset which will lead to significant binary
+// size bloat when imported into production code.
+package dcmtest

pkg/dcmtest/examples_test.go

@@ -0,0 +1,32 @@
+package dcmtest_test
+
+import (
+	"github.com/suyashkumar/dicom"
+	"github.com/suyashkumar/dicom/pkg/dcmtest"
+	"testing"
+)
+
+func MyDatasetFunc(dataset dicom.Dataset) error {
+	return nil
+}
+
+func ExampleWalkIncludedFS() {
+	// For this example we will make a mock *testing.T value. Normally this would be
+	// passed into your testing function automatically by `go test`.
+	t := new(testing.T)
+
+	// Each file in dcmtest.IncludedFS will be run in it's own subtest automatically.
+	dcmtest.WalkIncludedFS(t, func(t *testing.T, tc dcmtest.FSTestCase, setupErr error) {
+		// On a setup error, we want to report it. setupErr is not reported to t
+		// automatically, and is left to the caller to handle.
+		if setupErr != nil {
+			t.Fatal("setup error:", setupErr)
+		}
+
+		// Pass the pared dataset to the function we actually want to test
+		err := MyDatasetFunc(tc.Dataset)
+		if err != nil {
+			t.Error("MyDatasetFunc error:", err)
+		}
+	})
+}

pkg/dcmtest/fswalk.go

@@ -0,0 +1,147 @@
+package dcmtest
+
+import (
+	"fmt"
+	"github.com/suyashkumar/dicom"
+	"io/fs"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+)
+
+// walkFS backs WalkAndTestFS and WalkAndBenchFS.
+func walkFS(
+	tb testing.TB,
+	dcmFS fs.FS,
+	testFunc func(t *testing.T, tc FSTestCase, setupErr error),
+	benchFunc func(t *testing.B, tc FSTestCase, setupErr error),
+) {
+	// Walk the testdata directory.
+	_ = fs.WalkDir(dcmFS, ".", func(path string, d fs.DirEntry, err error) error {
+		// Only test "*.dcm" files.
+		if d.IsDir() || strings.ToLower(filepath.Ext(d.Name())) != ".dcm" {
+			return nil
+		}
+
+		// For each non-directory file that ends in '.dcm', run a test or bench.
+		switch runner := tb.(type) {
+		case *testing.T:
+			runner.Run(path, func(t *testing.T) {
+				// Set up our test case.
+				tc, walkErr := newWalkTestCase(t, dcmFS, path)
+				// Call the supplied testing function.
+				testFunc(t, tc, walkErr)
+			})
+		case *testing.B:
+			runner.Run(path, func(b *testing.B) {
+				// Set up our test case.
+				tc, walkErr := newWalkTestCase(b, dcmFS, path)
+				// Call the supplied testing function.
+				benchFunc(b, tc, walkErr)
+			})
+		default:
+			tb.Fatalf(
+				"runner was not *testing.T or *testing.B, got unsupported type: %v",
+				reflect.TypeOf(tb),
+			)
+		}
+
+		return nil
+	})
+}
+
+// WalkFS walks the test *.dcm files found in dcmFS and runs testFunc against each file.
+//
+// Each entry ending in ".dcm" will be run in it's own subtest named after it's path
+//
+// Setup errors will not be logged to t automatically. testFunc must decide whether and
+// how to log them itself.
+//
+// This package comes with a limited set of testdata stored in IncludedFS, if you
+// wish to run tests against these files, you can use WalkIncludedFS instead.
+//
+// Subtests are not run in parallel by default. Users must call t.Parallel in their
+// test functions to enable parallel testing.
+func WalkFS(
+	t *testing.T,
+	dcmFS fs.FS,
+	testFunc func(t *testing.T, tc FSTestCase, setupErr error),
+) {
+	walkFS(t, dcmFS, testFunc, nil)
+}
+
+// BenchFS is as WalkFS but takes *testing.B instead of *testing.T for benchmarking.
+func BenchFS(
+	b *testing.B,
+	dcmFS fs.FS,
+	benchFunc func(b *testing.B, tc FSTestCase, setupErr error),
+) {
+	walkFS(b, dcmFS, nil, benchFunc)
+}
+
+// FSTestCase is a testcase for a single dicom file.
+type FSTestCase struct {
+	// DCMPath is the full source file path of DCMFile. This value will always be
+	// populated, even if testFunc is passed a non-nil setupErr.
+	DCMPath string
+
+	// DCMFile is the Dicom file for testing. This file will be automatically closed on
+	// test cleanup.
+	DCMFile fs.File
+
+	// DCMStat is the result of calling DCMFile.Info()
+	DCMStat fs.FileInfo
+
+	// Dataset is the DICOM dataset parsed from DCMFile.
+	Dataset dicom.Dataset
+}
+
+// newWalkTestCase sets up a new test case by parsing the source file, then re-writing
+// and re-parsing the dataset.
+func newWalkTestCase(tb testing.TB, dcmFS fs.FS, sourcePath string) (FSTestCase, error) {
+	tc := FSTestCase{DCMPath: sourcePath}
+
+	// Load the source file into the test case.
+	file, err := dcmFS.Open(sourcePath)
+	if err != nil {
+		return tc, fmt.Errorf("error opening file '%v': %w", sourcePath, err)
+	}
+
+	// Register the file to be closed on test completion.
+	tb.Cleanup(func() {
+		closeErr := tc.DCMFile.Close()
+		if closeErr != nil {
+			tb.Errorf("error closing file '%v': %v", sourcePath, closeErr)
+		}
+	})
+	tc.DCMFile = file
+
+	tc.DCMStat, err = tc.DCMFile.Stat()
+	if err != nil {
+		return tc, fmt.Errorf("error getting file stat for '%v': %w", sourcePath, err)
+	}
+
+	tc.Dataset, err = dicom.Parse(tc.DCMFile, tc.DCMStat.Size(), nil)
+	if err != nil {
+		return tc, fmt.Errorf("error parsing dataset for '%v': %w", sourcePath, err)
+	}
+
+	return tc, nil
+}
+
+// WalkIncludedFS works as WalkFS, but runs against the included IncludedFS var.
+func WalkIncludedFS(
+	t *testing.T,
+	testFunc func(t *testing.T, tc FSTestCase, setupErr error),
+) {
+	WalkFS(t, IncludedFS, testFunc)
+}
+
+// BenchIncludedFS works as BenchFS, but runs against the included IncludedFS var.
+func BenchIncludedFS(
+	b *testing.B,
+	testFunc func(b *testing.B, tc FSTestCase, setupErr error),
+) {
+	BenchFS(b, IncludedFS, testFunc)
+}