Skip to content
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

Feature/dcmtest package #201

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bpeake-illuscio
Copy link
Contributor

@bpeake-illuscio bpeake-illuscio commented Apr 5, 2021

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!

This was referenced Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants