search

hammerlab / guacamole

Spark-based variant calling, with experimental support for multi-sample somatic calling (including RNA) and local assembly
Apache License 2.0
84 stars 21 forks source link

move makeRead from TestUtil to ReadsUtil #543

Closed ryan-williams closed 8 years ago

ryan-williams commented 8 years ago

also factor out utilities for making a reads RDD and TestRegions

This change is Reviewable

timodonnell commented 8 years ago

Does anything besides tests use makeRead? It seemed like not something we'd want to use elsewhere. But either way, LGTM

ryan-williams commented 8 years ago

Thanks for the review, but I'm am confused by your comment, so just to follow up:

Does anything besides tests use makeRead?

No.

It seemed like not something we'd want to use elsewhere.

It's not being used elsewhere, than… "tests", which is an important 1/3 of our codebase within which 12 files had 189 instances of a specific kind of boilerplate that they no longer have.

Writing good, clear tests is really hard to do; having libraries that abstract common testing infrastructure is a necessity, as is getting setup-boilerplate out of the way of readers seeking to understand the data input and expected/verified data output, which is the spirit behind this change and several others present and future.

makeRead already existed in one such library, TestUtil. I just moved it to its own mix-in trait, which is also one way to allow for calling makeRead instead of TestUtil.makeRead in 189 places.

timodonnell commented 8 years ago

My question was trying to understand whether this was being moved out of TestUtil because you wanted to use it in non-test code later, or if it was intended just as a refactoring of the testing code. Sounds like it's the latter, thanks for clarifying.

ryan-williams commented 8 years ago

yea, putting test-setup code pertaining to lots of different data-models (reads, regions, rdds of reads, pileups, references) in one file called TestUtil is an anti-pattern. Tests, like non-test code, should depend on exactly what they use, and no more, to the extent possible/practical.