Feature/dcmtest package

[bpeake-illuscio]

Hello!

Following up on #198, I started writing a standalone PR for the full-DICOM element equality roundtrip tests, and realized that some of the helpers I was building might be useful for public consumption (for instance, I would want them for testing some things that I am building). I always appreciate libs that expose test helpers where appropriate, and this seems like a place where exposing some internal helpers could possibly benefit other maintainers.

This is kind of a throw-something-new-against-the-wall Draft PR and see if it sticks. I totally understand if this isn't something you feel belongs in the lib.

Changes Overview

dcmtest Package Added

I've added a /pkg/dcmtest package and moved the old /testdata folder into this package as a /pkg/dcmtest/data sub-folder.

Inside the dcmtest package, Ive used the new go 1.16 embed package to add a dcmtest.IncludedFS var that contains an embed.FS FS value which is an embedded filesystem containing all of our test files, so other packages can pull in our test files and use them in their own tests without worrying about finnicky test file paths. In theory, this should also make tests a teensy bit more performant as the test files may be kept in memory instead of read from disk each time, but I see this more as a collateral benefit than the driving purpose of ease-of-use for writing tests that consume real dicom files.

This package also has a few test helpers like dcmtest.WalkIncludedFS and dcmtest.BenchIncludedFS which can be used to automatically setup and run table tests against every DICOM file contained in dcmtest.IncludedFS.

Here is an example of how that might look, using a re-written parse test from parse_test.go, which is also included in this PR:

// 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.
	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 /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) {
		// 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)
		}

		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++ {
			// 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,
				)
			}
		}
	})
}

I've also included a more general pair of dcmtest.WalkFS and dcmtest.BenchFS which take in an arbitrary fs.Fs interface for if we or others want to add or use a different FileSystem with other datasets for their table tests. I like this because, in theory, you could wrap a PACS server API or a DICOMDIR in a fs.FS implementation and pass it to this helper function for running table-tests against your dataset.

Bumped Go version to 1.16

I've altered the go.mod and Github Actions to use go 1.16 so we can gain access to the embed feature.

Possible Concerns.

There is one major concern with this approach, if dcmtest is imported into a non-test file, it could significantly bloat binary
sizes. I have tried to mitigate this possibility by adding warning sections to the package that spell out this package should only be consumed by tests.

I also tested this to double-check that it will not inadvertently bloat binaries and confirmed that no binary bloat occurs when this package is only consumed by test files, even if the test file is part of the production package instead of a package_test companion package.

Next Steps

I'm going to open a couple more PRs based on this for the pydicom and read/write roundtrip tests, but again. If you're not sure about this I am more than happy to redo those without these changes.

Let me know what you think!