From 9a141284b1814cb3820f2c676f91ca931e44387c Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Sun, 4 Apr 2021 15:25:20 -0700 Subject: [PATCH 01/16] setup test file, make command, and CI function --- .github/workflows/go.yml | 2 +- Makefile | 4 ++++ roundtrip_test.go | 4 ++++ 3 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 roundtrip_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ff8f59ad..d09c0c42 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -27,6 +27,6 @@ jobs: - name: Test run: | - make test + make test-full diff --git a/Makefile b/Makefile index 389c91a1..305aaa8c 100644 --- a/Makefile +++ b/Makefile @@ -15,6 +15,10 @@ build-fast: test: go test ./... -v +.PHONY: test-full +full: + go test ./... -v tags=integration + .PHONY: run run: make build diff --git a/roundtrip_test.go b/roundtrip_test.go new file mode 100644 index 00000000..5f309312 --- /dev/null +++ b/roundtrip_test.go @@ -0,0 +1,4 @@ +// +build integration + +package dicom + From ae9b061c158efb880c64f1b056338cb9f4bae61f Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 13:21:28 -0700 Subject: [PATCH 02/16] first pass at dcmtest package --- .github/workflows/bench_pr.yml | 4 +- Makefile | 6 +- go.mod | 2 +- parse_example_test.go | 52 +++++++ parse_test.go | 112 ++----------- pkg/dcmtest/data.go | 16 ++ {testdata => pkg/dcmtest/data}/1.dcm | Bin {testdata => pkg/dcmtest/data}/2.dcm | Bin {testdata => pkg/dcmtest/data}/3.dcm | Bin {testdata => pkg/dcmtest/data}/4.dcm | Bin {testdata => pkg/dcmtest/data}/5.dcm | Bin {testdata => pkg/dcmtest}/data_details.md | 0 pkg/dcmtest/doc.go | 3 + pkg/dcmtest/fswalk.go | 147 ++++++++++++++++++ pkg/dcmtest/fswalk_test.go | 69 ++++++++ .../dcmtest}/included_licenses.md | 0 roundtrip_test.go | 4 - 17 files changed, 309 insertions(+), 106 deletions(-) create mode 100644 parse_example_test.go create mode 100644 pkg/dcmtest/data.go rename {testdata => pkg/dcmtest/data}/1.dcm (100%) rename {testdata => pkg/dcmtest/data}/2.dcm (100%) rename {testdata => pkg/dcmtest/data}/3.dcm (100%) rename {testdata => pkg/dcmtest/data}/4.dcm (100%) rename {testdata => pkg/dcmtest/data}/5.dcm (100%) rename {testdata => pkg/dcmtest}/data_details.md (100%) create mode 100644 pkg/dcmtest/doc.go create mode 100644 pkg/dcmtest/fswalk.go create mode 100644 pkg/dcmtest/fswalk_test.go rename {testdata => pkg/dcmtest}/included_licenses.md (100%) delete mode 100644 roundtrip_test.go diff --git a/.github/workflows/bench_pr.yml b/.github/workflows/bench_pr.yml index bbfc8a7e..0348d5fc 100644 --- a/.github/workflows/bench_pr.yml +++ b/.github/workflows/bench_pr.yml @@ -25,11 +25,11 @@ jobs: go get golang.org/x/perf/cmd/benchstat echo "New Commit:" git log -1 --format="%H" - go test -bench=. -benchtime=.75s -count=8 > $HOME/new.txt + go test -tags=dicom_test_data -bench=. -benchtime=.75s -count=8 > $HOME/new.txt git reset --hard HEAD git checkout $GITHUB_BASE_REF echo "Base Commit:" git log -1 --format="%H" - go test -bench=. -benchtime=.75s -count=8 > $HOME/old.txt + go test -tags=dicom_test_data -bench=. -benchtime=.75s -count=8 > $HOME/old.txt benchstat $HOME/old.txt $HOME/new.txt diff --git a/Makefile b/Makefile index 305aaa8c..a7cfa11c 100644 --- a/Makefile +++ b/Makefile @@ -13,11 +13,11 @@ build-fast: .PHONY: test test: - go test ./... -v + go test ./... -v -tags dicom_test_data .PHONY: test-full -full: - go test ./... -v tags=integration +test-full: + go test ./... -v -tags=dicom_test_data .PHONY: run run: diff --git a/go.mod b/go.mod index de5b8a0c..ec6de702 100644 --- a/go.mod +++ b/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 diff --git a/parse_example_test.go b/parse_example_test.go new file mode 100644 index 00000000..8bd54d1a --- /dev/null +++ b/parse_example_test.go @@ -0,0 +1,52 @@ +package dicom_test + +import ( + "encoding/json" + "fmt" + "github.com/suyashkumar/dicom" + "github.com/suyashkumar/dicom/pkg/frame" + "github.com/suyashkumar/dicom/pkg/tag" + "image/jpeg" + "os" +) + +func Example_readFile() { + // See also: dicom.Parse, which uses a more generic io.Reader API. + dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil) + + // Dataset will nicely print the DICOM dataset data out of the box. + fmt.Println(dataset) + + // Dataset is also JSON serializable out of the box. + j, _ := json.Marshal(dataset) + fmt.Println(j) +} + +func Example_streamingFrames() { + frameChan := make(chan *frame.Frame) + + // Go routine to handle streaming frames as they are parsed. This may be + // useful when parsing a large DICOM with many frames from a network source, + // where image frames can start to be processed before the entire DICOM + // is parsed (or even read from storage). + go func() { + for fr := range frameChan { + fmt.Println(fr) + } + }() + + dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", frameChan) + fmt.Println(dataset) // The full dataset +} + +func Example_getImageFrames() { + 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 { + img, _ := fr.GetImage() // The Go image.Image for this frame + f, _ := os.Create(fmt.Sprintf("image_%d.jpg", i)) + _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 100}) + _ = f.Close() + } +} diff --git a/parse_test.go b/parse_test.go index fd30cf80..6611dd24 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1,116 +1,36 @@ package dicom_test import ( - "bytes" - "encoding/json" - "fmt" - "image/jpeg" - "io/ioutil" - "os" - "strings" + "github.com/suyashkumar/dicom/pkg/dcmtest" "testing" - "github.com/suyashkumar/dicom/pkg/tag" - - "github.com/suyashkumar/dicom/pkg/frame" - "github.com/suyashkumar/dicom" ) // 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. 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. 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) - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - _, _ = dicom.Parse(bytes.NewBuffer(data), int64(len(data)), nil) - } - }) + 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) } - } -} - -func Example_readFile() { - // See also: dicom.Parse, which uses a more generic io.Reader API. - dataset, _ := dicom.ParseFile("testdata/1.dcm", nil) - - // Dataset will nicely print the DICOM dataset data out of the box. - fmt.Println(dataset) - // Dataset is also JSON serializable out of the box. - j, _ := json.Marshal(dataset) - fmt.Println(j) -} - -func Example_streamingFrames() { - frameChan := make(chan *frame.Frame) - - // Go routine to handle streaming frames as they are parsed. This may be - // useful when parsing a large DICOM with many frames from a network source, - // where image frames can start to be processed before the entire DICOM - // is parsed (or even read from storage). - go func() { - for fr := range frameChan { - fmt.Println(fr) + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, _ = dicom.Parse(tc.DCMFile, tc.DCMStat.Size(), nil) } - }() - - dataset, _ := dicom.ParseFile("testdata/1.dcm", frameChan) - fmt.Println(dataset) // The full dataset -} - -func Example_getImageFrames() { - dataset, _ := dicom.ParseFile("testdata/1.dcm", nil) - pixelDataElement, _ := dataset.FindElementByTag(tag.PixelData) - pixelDataInfo := dicom.MustGetPixelDataInfo(pixelDataElement.Value) - for i, fr := range pixelDataInfo.Frames { - img, _ := fr.GetImage() // The Go image.Image for this frame - f, _ := os.Create(fmt.Sprintf("image_%d.jpg", i)) - _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 100}) - _ = f.Close() - } + }) } diff --git a/pkg/dcmtest/data.go b/pkg/dcmtest/data.go new file mode 100644 index 00000000..74a000a8 --- /dev/null +++ b/pkg/dcmtest/data.go @@ -0,0 +1,16 @@ +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. +// +// WARNING: this var should only be used for testing purposes. It contains a number of +// full DICOM files that could significantly bloat a binary if included in production +// code. +// +//go:embed **/*.dcm +var IncludedFS embed.FS diff --git a/testdata/1.dcm b/pkg/dcmtest/data/1.dcm similarity index 100% rename from testdata/1.dcm rename to pkg/dcmtest/data/1.dcm diff --git a/testdata/2.dcm b/pkg/dcmtest/data/2.dcm similarity index 100% rename from testdata/2.dcm rename to pkg/dcmtest/data/2.dcm diff --git a/testdata/3.dcm b/pkg/dcmtest/data/3.dcm similarity index 100% rename from testdata/3.dcm rename to pkg/dcmtest/data/3.dcm diff --git a/testdata/4.dcm b/pkg/dcmtest/data/4.dcm similarity index 100% rename from testdata/4.dcm rename to pkg/dcmtest/data/4.dcm diff --git a/testdata/5.dcm b/pkg/dcmtest/data/5.dcm similarity index 100% rename from testdata/5.dcm rename to pkg/dcmtest/data/5.dcm diff --git a/testdata/data_details.md b/pkg/dcmtest/data_details.md similarity index 100% rename from testdata/data_details.md rename to pkg/dcmtest/data_details.md diff --git a/pkg/dcmtest/doc.go b/pkg/dcmtest/doc.go new file mode 100644 index 00000000..fd0be8a1 --- /dev/null +++ b/pkg/dcmtest/doc.go @@ -0,0 +1,3 @@ +// Package dcmtest contains helper methods and functions for testing against dicom +// files. +package dcmtest diff --git a/pkg/dcmtest/fswalk.go b/pkg/dcmtest/fswalk.go new file mode 100644 index 00000000..8b3d4d79 --- /dev/null +++ b/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) +} diff --git a/pkg/dcmtest/fswalk_test.go b/pkg/dcmtest/fswalk_test.go new file mode 100644 index 00000000..da852cd4 --- /dev/null +++ b/pkg/dcmtest/fswalk_test.go @@ -0,0 +1,69 @@ +// +build dicom_test_data + +package dcmtest_test + +import ( + "github.com/suyashkumar/dicom/pkg/dcmtest" + "io/fs" + "strings" + "testing" +) + +// TestDCMTestFiles tests that our files are being embedded correctly. +func TestDCMTestFiles(t *testing.T) { + dcmCount := 0 + + err := fs.WalkDir(dcmtest.IncludedFS, ".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + t.Errorf("error walking directory path '%v': %v", path, err) + return err + } + + t.Log("FOUND:", d.Name()) + + if d.IsDir() { + return nil + } + + if !strings.HasSuffix(path, ".dcm") { + t.Errorf("non-dcm file found: '%v'", path) + } + + dcmCount++ + + return nil + }) + + if err != nil { + t.Errorf("error walking test file tree: %v", err) + } + + // Check that we have at least one .dcm file. + if dcmCount == 0 { + t.Errorf("no dcm files embedded") + } +} + +// TestWalkDCMFiles tests our test helper. +func TestWalkDCMFiles(t *testing.T) { + testsCount := 0 + + dcmtest.WalkIncludedFS(t, func(t *testing.T, tc dcmtest.FSTestCase, setupErr error) { + t.Log("FILE:", tc.DCMPath) + + if setupErr != nil { + t.Fatalf("error with entry: %v", setupErr) + } + + if !strings.HasSuffix(tc.DCMPath, ".dcm") { + t.Errorf("non-dcm file found: '%v'", tc.DCMPath) + } + + testsCount++ + }) + + // Check that we have at least one .dcm file. + if testsCount == 0 { + t.Errorf("no dcm files embedded") + } +} diff --git a/testdata/included_licenses.md b/pkg/dcmtest/included_licenses.md similarity index 100% rename from testdata/included_licenses.md rename to pkg/dcmtest/included_licenses.md diff --git a/roundtrip_test.go b/roundtrip_test.go deleted file mode 100644 index 5f309312..00000000 --- a/roundtrip_test.go +++ /dev/null @@ -1,4 +0,0 @@ -// +build integration - -package dicom - From 11b51bdf4413f78f69c302916464991fa8dfe057 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 14:04:49 -0700 Subject: [PATCH 03/16] removed erroneous build tag --- go.sum | 5 +++++ pkg/dcmtest/fswalk_test.go | 2 -- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/go.sum b/go.sum index ad35aa32..240423cd 100644 --- a/go.sum +++ b/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= diff --git a/pkg/dcmtest/fswalk_test.go b/pkg/dcmtest/fswalk_test.go index da852cd4..08ef5b37 100644 --- a/pkg/dcmtest/fswalk_test.go +++ b/pkg/dcmtest/fswalk_test.go @@ -1,5 +1,3 @@ -// +build dicom_test_data - package dcmtest_test import ( From b7c176581cf9ad9e97abf27f068845da84309bc4 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 14:37:02 -0700 Subject: [PATCH 04/16] added example --- pkg/dcmtest/examples_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 pkg/dcmtest/examples_test.go diff --git a/pkg/dcmtest/examples_test.go b/pkg/dcmtest/examples_test.go new file mode 100644 index 00000000..004427f3 --- /dev/null +++ b/pkg/dcmtest/examples_test.go @@ -0,0 +1,33 @@ +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. + if setupErr != nil { + t.Fatal("setup error:", setupErr) + } + + t.Log("TESTING:", tc.DCMPath) + + // Pass the pared dataset to the function we actually want to test + err := MyDatasetFunc(tc.Dataset) + if err != nil { + t.Error("MyDatasetFunc error:", err) + } + }) +} From 740645bae00017b9276938e2f744e8e00c27f6ef Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 14:40:47 -0700 Subject: [PATCH 05/16] bumped github actions to go 1.16 --- .github/workflows/bench_pr.yml | 4 ++-- .github/workflows/bench_push.yml | 4 ++-- .github/workflows/go.yml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/bench_pr.yml b/.github/workflows/bench_pr.yml index 0348d5fc..bb3ef1b5 100644 --- a/.github/workflows/bench_pr.yml +++ b/.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 diff --git a/.github/workflows/bench_push.yml b/.github/workflows/bench_push.yml index 22c037af..2c1eaa7a 100644 --- a/.github/workflows/bench_push.yml +++ b/.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 diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d09c0c42..b3584866 100644 --- a/.github/workflows/go.yml +++ b/.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 From ec681b8f59ae37d984459da684c9f29d15c7745a Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 14:42:39 -0700 Subject: [PATCH 06/16] reverted test runs to no build tags in makefile and github actions --- .github/workflows/bench_pr.yml | 4 ++-- .github/workflows/go.yml | 2 +- Makefile | 6 +----- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/.github/workflows/bench_pr.yml b/.github/workflows/bench_pr.yml index bb3ef1b5..6caa29ac 100644 --- a/.github/workflows/bench_pr.yml +++ b/.github/workflows/bench_pr.yml @@ -25,11 +25,11 @@ jobs: go get golang.org/x/perf/cmd/benchstat echo "New Commit:" git log -1 --format="%H" - go test -tags=dicom_test_data -bench=. -benchtime=.75s -count=8 > $HOME/new.txt + go test -bench=. -benchtime=.75s -count=8 > $HOME/new.txt git reset --hard HEAD git checkout $GITHUB_BASE_REF echo "Base Commit:" git log -1 --format="%H" - go test -tags=dicom_test_data -bench=. -benchtime=.75s -count=8 > $HOME/old.txt + go test -bench=. -benchtime=.75s -count=8 > $HOME/old.txt benchstat $HOME/old.txt $HOME/new.txt diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index b3584866..999a004f 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -27,6 +27,6 @@ jobs: - name: Test run: | - make test-full + make test diff --git a/Makefile b/Makefile index a7cfa11c..389c91a1 100644 --- a/Makefile +++ b/Makefile @@ -13,11 +13,7 @@ build-fast: .PHONY: test test: - go test ./... -v -tags dicom_test_data - -.PHONY: test-full -test-full: - go test ./... -v -tags=dicom_test_data + go test ./... -v .PHONY: run run: From 4c7e57a52cae1e361c25677d96cc81094821e4ae Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:00:58 -0700 Subject: [PATCH 07/16] updated test comments and moved data .MD files into dcmtest/data --- parse_test.go | 3 ++- pkg/dcmtest/{ => data}/data_details.md | 0 pkg/dcmtest/{ => data}/included_licenses.md | 0 3 files changed, 2 insertions(+), 1 deletion(-) rename pkg/dcmtest/{ => data}/data_details.md (100%) rename pkg/dcmtest/{ => data}/included_licenses.md (100%) diff --git a/parse_test.go b/parse_test.go index 6611dd24..4cc5ff9f 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1,10 +1,10 @@ package dicom_test import ( - "github.com/suyashkumar/dicom/pkg/dcmtest" "testing" "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 @@ -21,6 +21,7 @@ func TestParse(t *testing.T) { // BenchmarkParse runs sanity benchmarks over the sample files in testdata. func BenchmarkParse(b *testing.B) { + // 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. diff --git a/pkg/dcmtest/data_details.md b/pkg/dcmtest/data/data_details.md similarity index 100% rename from pkg/dcmtest/data_details.md rename to pkg/dcmtest/data/data_details.md diff --git a/pkg/dcmtest/included_licenses.md b/pkg/dcmtest/data/included_licenses.md similarity index 100% rename from pkg/dcmtest/included_licenses.md rename to pkg/dcmtest/data/included_licenses.md From 46e40ff0a19f4439f2b8b60e6e2561832bcc7c1c Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:11:57 -0700 Subject: [PATCH 08/16] madde warnings their own hheadings --- pkg/dcmtest/data.go | 14 ++++++++------ pkg/dcmtest/doc.go | 6 ++++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/dcmtest/data.go b/pkg/dcmtest/data.go index 74a000a8..f74a7b67 100644 --- a/pkg/dcmtest/data.go +++ b/pkg/dcmtest/data.go @@ -4,13 +4,15 @@ 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. +// 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 if included in production -// code. +// 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 diff --git a/pkg/dcmtest/doc.go b/pkg/dcmtest/doc.go index fd0be8a1..9fb094e7 100644 --- a/pkg/dcmtest/doc.go +++ b/pkg/dcmtest/doc.go @@ -1,3 +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 From 89179c485e216b781295735e18fc04435f4986c0 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:16:50 -0700 Subject: [PATCH 09/16] examples tweaks --- pkg/dcmtest/examples_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/dcmtest/examples_test.go b/pkg/dcmtest/examples_test.go index 004427f3..7b092054 100644 --- a/pkg/dcmtest/examples_test.go +++ b/pkg/dcmtest/examples_test.go @@ -17,13 +17,12 @@ func ExampleWalkIncludedFS() { // 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. + // 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) } - t.Log("TESTING:", tc.DCMPath) - // Pass the pared dataset to the function we actually want to test err := MyDatasetFunc(tc.Dataset) if err != nil { From 86bfead678532fc53043512333b993ce20dc10b1 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:34:39 -0700 Subject: [PATCH 10/16] moved parse examples back to parse_test.go --- parse_example_test.go | 52 ------------------------------------------- parse_test.go | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 52 deletions(-) delete mode 100644 parse_example_test.go diff --git a/parse_example_test.go b/parse_example_test.go deleted file mode 100644 index 8bd54d1a..00000000 --- a/parse_example_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package dicom_test - -import ( - "encoding/json" - "fmt" - "github.com/suyashkumar/dicom" - "github.com/suyashkumar/dicom/pkg/frame" - "github.com/suyashkumar/dicom/pkg/tag" - "image/jpeg" - "os" -) - -func Example_readFile() { - // See also: dicom.Parse, which uses a more generic io.Reader API. - dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil) - - // Dataset will nicely print the DICOM dataset data out of the box. - fmt.Println(dataset) - - // Dataset is also JSON serializable out of the box. - j, _ := json.Marshal(dataset) - fmt.Println(j) -} - -func Example_streamingFrames() { - frameChan := make(chan *frame.Frame) - - // Go routine to handle streaming frames as they are parsed. This may be - // useful when parsing a large DICOM with many frames from a network source, - // where image frames can start to be processed before the entire DICOM - // is parsed (or even read from storage). - go func() { - for fr := range frameChan { - fmt.Println(fr) - } - }() - - dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", frameChan) - fmt.Println(dataset) // The full dataset -} - -func Example_getImageFrames() { - 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 { - img, _ := fr.GetImage() // The Go image.Image for this frame - f, _ := os.Create(fmt.Sprintf("image_%d.jpg", i)) - _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 100}) - _ = f.Close() - } -} diff --git a/parse_test.go b/parse_test.go index 4cc5ff9f..237a9020 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1,12 +1,59 @@ package dicom_test import ( + "encoding/json" + "fmt" + "github.com/suyashkumar/dicom/pkg/frame" + "github.com/suyashkumar/dicom/pkg/tag" + "image/jpeg" + "os" "testing" "github.com/suyashkumar/dicom" "github.com/suyashkumar/dicom/pkg/dcmtest" ) +func Example_readFile() { + // See also: dicom.Parse, which uses a more generic io.Reader API. + dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil) + + // Dataset will nicely print the DICOM dataset data out of the box. + fmt.Println(dataset) + + // Dataset is also JSON serializable out of the box. + j, _ := json.Marshal(dataset) + fmt.Println(j) +} + +func Example_streamingFrames() { + frameChan := make(chan *frame.Frame) + + // Go routine to handle streaming frames as they are parsed. This may be + // useful when parsing a large DICOM with many frames from a network source, + // where image frames can start to be processed before the entire DICOM + // is parsed (or even read from storage). + go func() { + for fr := range frameChan { + fmt.Println(fr) + } + }() + + dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", frameChan) + fmt.Println(dataset) // The full dataset +} + +func Example_getImageFrames() { + 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 { + img, _ := fr.GetImage() // The Go image.Image for this frame + f, _ := os.Create(fmt.Sprintf("image_%d.jpg", i)) + _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 100}) + _ = f.Close() + } +} + // 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. func TestParse(t *testing.T) { From afd9fe185b246550a9bd7a6e4bb35c69b229e07a Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:35:27 -0700 Subject: [PATCH 11/16] moved parse examples back to original place --- parse_test.go | 58 +++++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/parse_test.go b/parse_test.go index 237a9020..38cb2ac8 100644 --- a/parse_test.go +++ b/parse_test.go @@ -13,6 +13,35 @@ import ( "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. +func TestParse(t *testing.T) { + // 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. +func BenchmarkParse(b *testing.B) { + // 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(tc.DCMFile, tc.DCMStat.Size(), nil) + } + }) +} + func Example_readFile() { // See also: dicom.Parse, which uses a more generic io.Reader API. dataset, _ := dicom.ParseFile("./pkg/dcmtest/data/1.dcm", nil) @@ -53,32 +82,3 @@ func Example_getImageFrames() { _ = f.Close() } } - -// 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. -func TestParse(t *testing.T) { - // 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. -func BenchmarkParse(b *testing.B) { - // 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(tc.DCMFile, tc.DCMStat.Size(), nil) - } - }) -} From b09eb0f8b87b483ef2487e1c1e1c445d7d2ffd01 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:37:45 -0700 Subject: [PATCH 12/16] updated parse test comments --- parse_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/parse_test.go b/parse_test.go index 38cb2ac8..dca77d4a 100644 --- a/parse_test.go +++ b/parse_test.go @@ -13,8 +13,8 @@ import ( "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) { // This will walk all of our test data and try parsing the Dataset, so we can simply // report if we get passed an error. @@ -25,7 +25,7 @@ func TestParse(t *testing.T) { }) } -// 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) { // 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) { From 6669f9fddda05d9a2b738ec3cf6ebe6b90603a0f Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:49:15 -0700 Subject: [PATCH 13/16] basic test setup --- roundtrip_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 roundtrip_test.go diff --git a/roundtrip_test.go b/roundtrip_test.go new file mode 100644 index 00000000..9576c0e1 --- /dev/null +++ b/roundtrip_test.go @@ -0,0 +1,44 @@ +package dicom_test + +import ( + "bytes" + "github.com/suyashkumar/dicom" + "github.com/suyashkumar/dicom/pkg/dcmtest" + "testing" +) + +// TestRoundTrip tests that we can read a dicom from disk, write the dicom back to a +// buffer, then read it back in and yield two identical datasets. +func TestRoundTrip(t *testing.T) { + dcmtest.WalkIncludedFS(t, func(t *testing.T, tc dcmtest.FSTestCase, setupErr error) { + // make a new buffer to write to + rtCase := RoundTripTestCase{ + FSTestCase: tc, + } + + buffer := bytes.NewBuffer(make([]byte, 0, tc.DCMStat.Size())) + err := dicom.Write( + buffer, + rtCase.Dataset, + dicom.SkipVRVerification(), + dicom.SkipValueTypeVerification(), + ) + if err != nil { + t.Fatalf("error writing dataset: %v", err) + } + + rtCase.WrittenDataset, err = dicom.Parse(buffer, int64(buffer.Len()), nil) + if err != nil { + t.Fatalf("error re-parsing written dataset: %v", err) + } + }) +} + +// RoundTripTestCase is a test case for TestRoundTrip. +type RoundTripTestCase struct { + // Embed the test case passed in by dcmtest.WalkIncludedFS. + dcmtest.FSTestCase + // WrittenDataset is the dataset resulting from writing FSTestCase.Dataset to a + // buffer and re-parsing it. + WrittenDataset dicom.Dataset +} From 3ea3548b643c6ed601b84ca69a1382b04885a02f Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 15:54:06 -0700 Subject: [PATCH 14/16] test complete --- roundtrip_test.go | 287 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 274 insertions(+), 13 deletions(-) diff --git a/roundtrip_test.go b/roundtrip_test.go index 9576c0e1..450c3c31 100644 --- a/roundtrip_test.go +++ b/roundtrip_test.go @@ -2,8 +2,12 @@ package dicom_test import ( "bytes" + "fmt" + "github.com/google/go-cmp/cmp" "github.com/suyashkumar/dicom" "github.com/suyashkumar/dicom/pkg/dcmtest" + "github.com/suyashkumar/dicom/pkg/frame" + "github.com/suyashkumar/dicom/pkg/tag" "testing" ) @@ -11,15 +15,11 @@ import ( // buffer, then read it back in and yield two identical datasets. func TestRoundTrip(t *testing.T) { dcmtest.WalkIncludedFS(t, func(t *testing.T, tc dcmtest.FSTestCase, setupErr error) { - // make a new buffer to write to - rtCase := RoundTripTestCase{ - FSTestCase: tc, - } buffer := bytes.NewBuffer(make([]byte, 0, tc.DCMStat.Size())) err := dicom.Write( buffer, - rtCase.Dataset, + tc.Dataset, dicom.SkipVRVerification(), dicom.SkipValueTypeVerification(), ) @@ -27,18 +27,279 @@ func TestRoundTrip(t *testing.T) { t.Fatalf("error writing dataset: %v", err) } - rtCase.WrittenDataset, err = dicom.Parse(buffer, int64(buffer.Len()), nil) + writtenDataset, err := dicom.Parse(buffer, int64(buffer.Len()), nil) if err != nil { t.Fatalf("error re-parsing written dataset: %v", err) } + + // Compare the datasets and report discrepancies. + checkDatasets(t, tc.Dataset, writtenDataset) }) } -// RoundTripTestCase is a test case for TestRoundTrip. -type RoundTripTestCase struct { - // Embed the test case passed in by dcmtest.WalkIncludedFS. - dcmtest.FSTestCase - // WrittenDataset is the dataset resulting from writing FSTestCase.Dataset to a - // buffer and re-parsing it. - WrittenDataset dicom.Dataset +// checkDatasets checks that all written element values match the original. +func checkDatasets(t *testing.T, original, written dicom.Dataset) { + // If the number of elements in a dataset are mismatched, that's an immediate exit. + if len(original.Elements) != len(written.Elements) { + // TODO: add report of what elements are missing (or added) from/to the + // original. + t.Errorf( + "element count mis-match, original had %v, written has %v", + len(original.Elements), + len(written.Elements), + ) + } + + // Iterate over both datasets. + originalIter := original.FlatStatefulIterator() + writtenIter := written.FlatStatefulIterator() + + // We'll use the original dataset for our + for originalIter.HasNext() { + originalElm := originalIter.Next() + + // Get the element name for errors. Wll try to use the known name. + tagInfo, err := tag.Find(originalElm.Tag) + if err != nil { + // Default to the tag number if we don't know the tag. + tagInfo.Name = originalElm.Tag.String() + } + + if !writtenIter.HasNext() { + t.Errorf( + "could not get next element from written dataset on source"+ + " tag %v", + tagInfo.Name, + ) + } + writtenElm := writtenIter.Next() + + // Run a sub-test for this element. + t.Run(tagInfo.Name, func(t *testing.T) { + checkElement(t, originalElm, writtenElm) + }) + } +} + +// checkElement checks that two elements are identical. +func checkElement(t *testing.T, original, written *dicom.Element) { + t.Log("VR:", original.RawValueRepresentation) + + if original.Tag != written.Tag { + t.Fatalf( + "element tag mismatch. original is '%v', written is '%v'", + original.Tag, + written.Tag, + ) + } + + if original.RawValueRepresentation != written.RawValueRepresentation { + t.Errorf( + "element vr mismatch. original is '%v', written is '%v'", + original.RawValueRepresentation, + written.RawValueRepresentation, + ) + } + + if original.ValueRepresentation != written.ValueRepresentation { + t.Errorf( + "element vrkind mismatch. original is '%v', written is '%v'", + original.ValueRepresentation, + written.ValueRepresentation, + ) + } + + if original.ValueLength != written.ValueLength { + t.Errorf( + "element value length mismatch. original is '%v', written is '%v'", + original.ValueLength, + written.ValueLength, + ) + } + + // We need to handle SQ (sequence) and PixelData specially. + switch original.ValueRepresentation { + case tag.VRSequence: + originalSeq := original.Value.GetValue().([]*dicom.SequenceItemValue) + writtenSeq := written.Value.GetValue().([]*dicom.SequenceItemValue) + checkSequence(t, originalSeq, writtenSeq) + case tag.VRPixelData: + originalData := original.Value.GetValue().(dicom.PixelDataInfo) + writtenData := original.Value.GetValue().(dicom.PixelDataInfo) + checkPixelData(t, originalData, writtenData) + default: + // Otherwise we will just compare the values directly. + if !cmp.Equal(original.Value.GetValue(), written.Value.GetValue()) { + t.Errorf( + "original value does not equal written value. Diff: %v", + cmp.Diff(original.Value.GetValue(), written.Value.GetValue()), + ) + } + } +} + +// checkSequence checks that two sequence elements are identical. +func checkSequence(t *testing.T, original, written []*dicom.SequenceItemValue) { + // Because we are using a flat iterator in the main function, we don't need to + // deeply compare each element within the sequences. Just that the sequences + // have the same number of elements in the same order. + if len(original) != len(written) { + t.Fatalf( + "sequence item count mismatch. original: %v, written: %v", + len(original), + len(written), + ) + } + + for i, thisOriginal := range original { + thisWritten := written[i] + + originalElements := thisOriginal.GetValue().([]*dicom.Element) + writtenElements := thisWritten.GetValue().([]*dicom.Element) + + if len(originalElements) != len(writtenElements) { + t.Fatalf( + "element count mismatch for SequenceItemValue %v. original: %v, written: %v", + i, + len(originalElements), + len(writtenElements), + ) + } + + // Iterate over both sequences. + for j, originalElm := range originalElements { + writtenElm := writtenElements[j] + + // We just need to make sure the tags are equal to ensure ordering. + if originalElm.Tag != writtenElm.Tag { + t.Errorf( + "SQ value %v, element %v tag mismatch. original: %v, written: %v", + i, + j, + originalElm.Tag, + writtenElm.Tag, + ) + } + } + } +} + +// checkPixelData checks that pixel data values match +func checkPixelData(t *testing.T, original, written dicom.PixelDataInfo) { + if !original.IsEncapsulated == written.IsEncapsulated { + t.Errorf( + "PixelDataInfo.IsEncapsulated mismatch. original: %v written: %v", + original.IsEncapsulated, + written.IsEncapsulated, + ) + } + + if len(original.Offsets) != len(written.Offsets) { + t.Errorf( + "PixelDataInfo.Offsets length mismatch. original: %v written: %v", + len(original.Offsets), + len(written.Offsets), + ) + } + + if len(original.Frames) != len(written.Frames) { + // If the frame count is wong, we don't need to compare the frames. + t.Fatalf( + "PixelDataInfo.Frame count mismatch. original: %v written: %v", + len(original.Frames), + len(written.Frames), + ) + } + + for i, originalFrame := range original.Frames { + writtenFrame := written.Frames[i] + t.Run(fmt.Sprint("Frame", i), func(t *testing.T) { + checkFrame(t, originalFrame, writtenFrame) + }) + } +} + +// checkFrame checks that the written frame matches the original frame. +func checkFrame(t *testing.T, original, written frame.Frame) { + if original.Encapsulated != written.Encapsulated { + t.Errorf( + "Encapsulated mismatch. Original: %v Written: %v", + original.Encapsulated, + written.Encapsulated, + ) + } + + if original.Encapsulated { + originalData := original.EncapsulatedData.Data + writtenData := written.EncapsulatedData.Data + + if len(originalData) != len(writtenData) { + t.Fatalf( + "oiginal data length %v does not match written lenfth %v", + len(originalData), + len(writtenData), + ) + } + return + } + + if original.NativeData.BitsPerSample != written.NativeData.BitsPerSample { + t.Errorf( + "BitsPerSample mismatch. Original: %v Written: %v", + original.NativeData.BitsPerSample, + written.NativeData.BitsPerSample, + ) + } + + if original.NativeData.Cols != written.NativeData.Cols { + t.Errorf( + "Cols mismatch. Original: %v Written: %v", + original.NativeData.Cols, + written.NativeData.Cols, + ) + } + + if original.NativeData.Rows != written.NativeData.Rows { + t.Errorf( + "Rows mismatch. Original: %v Written: %v", + original.NativeData.Rows, + written.NativeData.Rows, + ) + } + + if len(original.NativeData.Data) != len(written.NativeData.Data) { + t.Fatalf( + "Pixel count mismatch. Original: %v Written %v", + len(original.NativeData.Data), + len(written.NativeData.Data), + ) + } + + // Check that all the pixel-values match. + for i, originalPixel := range original.NativeData.Data { + writtenPixel := written.NativeData.Data[i] + + if len(originalPixel) != len(writtenPixel) { + t.Fatalf( + "Pixel value count mismatch on pixel %v. oiginal: %v witten: %v", + i, + len(originalPixel), + len(writtenPixel), + ) + } + + for i2, originalValue := range originalPixel { + writtenValue := writtenPixel[i2] + + if originalValue != writtenValue { + t.Fatalf( + "pixel value mismatch for pixel %v, value %v. Original: %v, Written: %v", + i, + i2, + originalValue, + writtenValue, + ) + } + } + } } From cd959c9eea8f668f5e4035f3ba11c50870430f51 Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 18:12:52 -0700 Subject: [PATCH 15/16] fixed BenchmarkParse and altered FSTestCase to have open dcm file method --- parse_test.go | 37 ++++++++++++++++++++++++++++++++++++- pkg/dcmtest/fswalk.go | 36 ++++++++++++++++++------------------ 2 files changed, 54 insertions(+), 19 deletions(-) diff --git a/parse_test.go b/parse_test.go index dca77d4a..badafd43 100644 --- a/parse_test.go +++ b/parse_test.go @@ -1,11 +1,13 @@ package dicom_test import ( + "bytes" "encoding/json" "fmt" "github.com/suyashkumar/dicom/pkg/frame" "github.com/suyashkumar/dicom/pkg/tag" "image/jpeg" + "io" "os" "testing" @@ -35,9 +37,42 @@ func BenchmarkParse(b *testing.B) { b.Fatalf("setup error: %v", setupErr) } + dcmFile, err := tc.OpenDCMFile() + if err != nil { + b.Fatalf("error opening dcm file: %v", err) + } + defer dcmFile.Close() + + // Extract the binary data. + binData, err := io.ReadAll(dcmFile) + if err != nil { + b.Fatalf("error reading dcm file: %v", err) + } + b.ResetTimer() for i := 0; i < b.N; i++ { - _, _ = dicom.Parse(tc.DCMFile, tc.DCMStat.Size(), nil) + // Read from a fresh buffer each iteration. + buffer := bytes.NewBuffer(binData) + + // Parse the dataset. + dataset, err := dicom.Parse(buffer, tc.DCMStat.Size(), nil) + if err != nil { + b.Fatalf("error parsing dataset on repetition %v: %v", i, err) + } + + // Make sure we got sane results. + if len(dataset.Elements) == 0 { + b.Fatalf("no elements were parsed on repetition %v", i) + } + + if len(dataset.Elements) != len(tc.Dataset.Elements) { + b.Fatalf( + "element count mismatch: expected %v, got %v on repetition %v", + len(tc.Dataset.Elements), + len(dataset.Elements), + i, + ) + } } }) } diff --git a/pkg/dcmtest/fswalk.go b/pkg/dcmtest/fswalk.go index 8b3d4d79..b98c8fd8 100644 --- a/pkg/dcmtest/fswalk.go +++ b/pkg/dcmtest/fswalk.go @@ -82,47 +82,47 @@ func BenchFS( // FSTestCase is a testcase for a single dicom file. type FSTestCase struct { + // dcmFs holds our test filesystem. + dcmFs fs.FS + // 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 is the result of calling fs.File.Info() on the DCMPath. DCMStat fs.FileInfo - // Dataset is the DICOM dataset parsed from DCMFile. + // Dataset is the DICOM dataset parsed from the fs.File at DCMPath. Dataset dicom.Dataset } +// OpenDCMFile opens a new fs.File handler using DCMPath and the test fs.FS. +func (fsCase FSTestCase) OpenDCMFile() (fs.File, error) { + return fsCase.dcmFs.Open(fsCase.DCMPath) +} + // 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} + tc := FSTestCase{ + dcmFs: dcmFS, + DCMPath: sourcePath, + } // Load the source file into the test case. - file, err := dcmFS.Open(sourcePath) + file, err := tc.OpenDCMFile() 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 + defer file.Close() - tc.DCMStat, err = tc.DCMFile.Stat() + tc.DCMStat, err = file.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) + tc.Dataset, err = dicom.Parse(file, tc.DCMStat.Size(), nil) if err != nil { return tc, fmt.Errorf("error parsing dataset for '%v': %w", sourcePath, err) } From 17ce0bec341b3279af9ff0ad90bfd158185e3e6b Mon Sep 17 00:00:00 2001 From: Billy Peake Date: Mon, 5 Apr 2021 18:13:07 -0700 Subject: [PATCH 16/16] gofmt --- pkg/dcmtest/fswalk.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/dcmtest/fswalk.go b/pkg/dcmtest/fswalk.go index b98c8fd8..fc5eb06a 100644 --- a/pkg/dcmtest/fswalk.go +++ b/pkg/dcmtest/fswalk.go @@ -105,7 +105,7 @@ func (fsCase FSTestCase) OpenDCMFile() (fs.File, error) { // and re-parsing the dataset. func newWalkTestCase(tb testing.TB, dcmFS fs.FS, sourcePath string) (FSTestCase, error) { tc := FSTestCase{ - dcmFs: dcmFS, + dcmFs: dcmFS, DCMPath: sourcePath, }