Fix @BeforeSuite usages to prevent excessive code execution #3551
Conversation
| @@ -16,7 +16,7 @@ | |||
|
|
|||
|
|
|||
| public final class QualQuantizerUnitTest extends BaseTest { | |||
| @BeforeSuite | |||
| @BeforeClass | |||
| public void before() { | |||
There was a problem hiding this comment.
Hm, yeah, that should probably just come out. Even better.
| final ImmutablePair<List<String>, ReadCountCollection> randomData = generateRandomReadCountCollection( | ||
| @BeforeClass | ||
| public void before() { | ||
| HOMO_SAPIENS_GERMLINE_CONTIG_PLOIDY_ANNOTATIONS = ContigGermlinePloidyAnnotationTableReader |
There was a problem hiding this comment.
This seems worse, why change these?
There was a problem hiding this comment.
I did this because the static blocks always execute (AFAICT), even if you run a single test from another package, and it calls code that emits log output:
13:15:52.512 INFO ContigGermlinePloidyAnnotationTableReader - Ploidy tags: SEX_XX, SEX_XY
which looks misplaced depending on the context. But that is a separate problem - my main goal with this PR was to eliminate the BWA indexing, since thats super slow and it happens every time. I'm happy to revert the changes in this file for now.
| @@ -14,7 +14,7 @@ | |||
|
|
|||
| GenotypeLikelihoodCalculators calcs; | |||
|
|
|||
| @BeforeSuite | |||
| @BeforeClass | |||
| public void init(){ | |||
There was a problem hiding this comment.
I feel like most of these before suite should just be initialized inline where they're declared. Or replaced with a getter that returns a new instance of the thing on demand. That's a bigger change though.
There was a problem hiding this comment.
Agreed. I was trying to minimize the changes, but wanted to get rid of unnecessary @BeforeSuites.
|
|
||
| public class BwaMemIntegrationTest extends BaseTest { | ||
|
|
||
| private BwaMemIndex index; | ||
|
|
||
| @BeforeSuite | ||
| @BeforeClass | ||
| public void loadIndex() { |
There was a problem hiding this comment.
Does this really take minutes? that's gross... we should try to change it uses a smaller test file if that wouldn't reduce the power of the test unreasonably.
There was a problem hiding this comment.
It seems to take about 2 minutes on my laptop.
There was a problem hiding this comment.
I am suspecting this is leading to the new indexing step I see when running tests locally, and does increase the test time by ~3min.
There was a problem hiding this comment.
@cmnbroad A few minor comments. 👍 to merge whatever you decide to do with those. The before and after blocks for the most part seem like a bad idea to me in general, they tend to be very fragile. Getting rid of them is a bigger change though than just limiting the scope the run in though.
Codecov Report
@@ Coverage Diff @@
## master #3551 +/- ##
===============================================
+ Coverage 79.928% 79.938% +0.011%
- Complexity 17900 17901 +1
===============================================
Files 1199 1199
Lines 65015 65015
Branches 10124 10124
===============================================
+ Hits 51965 51972 +7
+ Misses 9016 9011 -5
+ Partials 4034 4032 -2
|
cmnbroad
force-pushed
the
cn_fix_test_suite_isolation
branch
from
c11f081 to
3868349
Compare
cmnbroad
changed the title
…ode execution when tests aren't included in a suite.
cmnbroad
force-pushed
the
cn_fix_test_suite_isolation
branch
from
3868349 to
788b48d
Compare
There is quite a bit of code annotated with @BeforeSuite that should probably just be @BeforeClass. @BeforeSuite code can run even when no methods in the class are selected in the current suite. Most of it is harmless, but some of it is painfully slow, i.e., using gradle to run a single test method always causes the BwaMemIntegrationTest code to create an index image for the large test reference, which takes 1-2 minutes, before the selected test even starts.
We should probably reserve @BeforeSuite for test-infrastructure setup code; most test classes should use @BeforeClass instead.