Enabling writing and running of tests with scalatest.

[tfenne]

Description

An experimental branch that enables writing and running of tests using scala test, in both Java and Scala. Making a PR so that I can test CI builds and code coverage.

Checklist

• Code compiles correctly
• New tests covering changes and new functionality
• All tests passing
• Extended the README / documentation, if necessary
• Is not backward compatible (breaks binary or source compatibility)

[codecov-io]

Codecov Report

Merging #789 into master will decrease coverage by 0.038%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master      #789       +/-   ##
===============================================
- Coverage     64.882%   64.844%   -0.038%     
+ Complexity      7198      7191        -7     
===============================================
  Files            527       527               
  Lines          31781     31781               
  Branches        5424      5424               
===============================================
- Hits           20620     20608       -12     
- Misses          9016      9035       +19     
+ Partials        2145      2138        -7

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/BAMFileSpan.java	27.083% <0%> (-44.792%)	11% <0%> (-20%)
.../main/java/htsjdk/samtools/fastq/FastqEncoder.java	48.276% <0%> (-34.483%)	7% <0%> (-5%)
...rc/main/java/htsjdk/samtools/SAMProgramRecord.java	67.568% <0%> (-8.108%)	13% <0%> (-2%)
...samtools/util/AsyncBlockCompressedInputStream.java	72% <0%> (-4%)	12% <0%> (-1%)
src/main/java/htsjdk/samtools/Chunk.java	81.667% <0%> (-3.333%)	27% <0%> (-1%)
...c/main/java/htsjdk/samtools/fastq/FastqRecord.java	82.609% <0%> (-2.899%)	30% <0%> (-3%)
.../main/java/htsjdk/samtools/SAMReadGroupRecord.java	48.276% <0%> (-1.724%)	16% <0%> (-1%)
...c/main/java/htsjdk/samtools/SAMSequenceRecord.java	67.073% <0%> (-1.22%)	30% <0%> (-1%)
...dk/samtools/seekablestream/SeekableHTTPStream.java	56.061% <0%> (+1.515%)	10% <0%> (+1%)	⬆️
src/main/java/htsjdk/tribble/TribbleException.java	55.556% <0%> (+5.556%)	5% <0%> (+1%)	⬆️
... and 2 more

[tfenne]

@lbergelson Would you be up for taking a look at this PR? Particularly with an eye towards the changes I've made in the gradle build? I'm not a gradle expert, and there may be better ways to do what I did, but I believe it is working now (with SRA tests being skipped locally and run on travis).

@yfarjoun & @nh13 want to cast an eye over this as well?

[yfarjoun]

why is this not extending HtsjdkTest? confused.

[tfenne]

Good catch. It should.

[yfarjoun]

why "UnitSpec"? if the java class is HtsjdkTest, shouldn't the scala version be close to that? perhaps HtsjdkScalaTest? Or at least HtsjdkSpec...

[tfenne]

The idea is that scalatest offers different styles of test writing, and one might choose to use one style for unit testing, and a different one for integration or larger functional testing. So the convention is to name the base class by the type of test it is intended for, in this case unit testing.

[yfarjoun]

I see. OK. thanks. I was confused since you used Htsjdk in the baseclass of the java tests

[yfarjoun]

should we request a default case here to assure that this isn't a silent failure? (I mean from a stylistic point of view..)

[yfarjoun]

(I could also not be understanding how scala works...)

[tfenne]

No. This is an example of a partial function (not to be confused with a partially applied function) in scala. It's the easiest/clearest way to do tuple-decomposition as part of an anonymous function invocation.

Also, it will fail if handed something it can't match and we can also see all the inputs to the left and see that they are three-tuples.

[yfarjoun]

thanks.

[yfarjoun]

why ${0}? 😕

${3} I would understand.

[yfarjoun]

also, why did you switch the order between the data and the test? everywhere else it's "test" in "data and here it's data => test.... I don't mind...I just want to understand if it matters...

[tfenne]

The ${0} is a mistake - should be ${distance}.
I'm not sure I understand your second comment.

[yfarjoun]

nm

[yfarjoun]

This seems to be a place where the reverse order makes more sence, since you are using the same data for two tests...wouldn't it make better sense to have

data.foreach(s=> test1; test2) ?

[yfarjoun]

ah, I now see that the data isn't exactly the same...NM.

[yfarjoun]

default case here?

[tfenne]

Same as above - it's normal not to have default cases in situations like this.

[yfarjoun]

ignore

[yfarjoun]

spaces after commas? (here and below)

[yfarjoun]

the original has a license...can we leave it on? (I know that many tests do not have license...)

[tfenne]

Sure.

[yfarjoun]

After all the discussion about how much better scala for writing tests, I was surprised that the tests didn't become much shorter...but the length of the code isn't the only factor. I think that this satisfies most (if not all) the requirements of the issue. but I think it would be good to finish that conversation in the issue and not here.

Once the small issues I raise here are dealt with, I'm fine with this PR, assuming that we are OK about scala for tests in htsjdk....but would need others to chime in..

I did NOT look at the specifics of the build system (gradle, jacoco, codecov, etc.)

[yfarjoun]

Thanks. this answers my issues. @lbergelson can you take a look at the changes to the build system?

[tfenne]

Thanks for the review @yfarjoun; I've pushed up some changes. I'd like to address your comment re: length of the tests. Maybe I shot myself in the foot there by adding a whole bunch more tests? Looking at the old vs. new I think that a fair count would put the equivalent set of tests (from class declaration through to the last ported test) at ~90 lines for Java and ~54 for Scala. So that's about 40% fewer lines of code, which is non-trivial.

I think what's important though is how easy it is to a) write the tests, b) understand them again quickly at a later date, c) add one more similar test case. I might also add d) how good the reporting is when a failure occurs. I find the scalatest version easier to read overall, and much easier to divine the intent of. I've never been a big fan of the TestNG style of separating dataProviders from the tests that use them, as it leaves you tracking back and forth across two places in a file trying to understand how some long array of objects is bound to arguments, etc. Having that close together makes it much easier to follow IMHO.

[yfarjoun]

@tfenne Thanks for pointing out the addition of new tests. I didn't see that originally.

I agree regarding readability, though I looked at the options for Matchers and was a little horrified at the differences between:

result should equal (3) // can customize equality
result should === (3)   // can customize equality and enforce type constraints
result should be (3)    // cannot customize equality, so fastest to compile
result shouldEqual 3    // can customize equality, no parentheses required
result shouldBe 3       // cannot customize equality, so fastest to compile, no parentheses required

Not that it isn't good to have all these matchers, but rather that it will be more difficult to write correct tests and review them.

[lbergelson]

I didn't know about this tags syntax, that's much better than the if-else chain

[lbergelson]

Could you add a comment about what this is and why it's needed?

[tfenne]

Will do. FYI it's used to generate the pretty HTML reports from scalatest.

[lbergelson]

We added this toggle here because the test output was growing so large that travis was failing.

It seems like travis is passing with this now, so they may have increased their log file size limit, or the new output is less verbose than it was. However, I think it's still valuable to have the cleaner output format on travis that only displays some progress indicator + failed/skipped tests. It makes it much easier to diagnose what went wrong at a glance.

[tfenne]

I don't disagree @lbergelson, though I've just spent the last two hours trying to make this work and am running out of ideas. It seems like the combination of gradle + scalatest + TestNG make this rather hard to figure out. The code that was in the build file previously doesn't seem to suppress much of anything (I changed the test to if (true) to test locally).

How critical do you think this is?

[lbergelson]

It's only critical if it starts killing travis runs again due to the verbosity. If it's a horrible pain to figure out (which I certainly imagine it could be... I'm not even sure what's generating test output now, is it scalatest, testNG being invoked by scalatest?) then I'd say it's fine as is. Thanks for looking into it. If it really becomes problematic on travis we can pipe the test output into dev/null or grep fail or something like that.

[lbergelson]

@tfenne I had a few minor comments about the build. I think it would be useful to maintain the minimal test output on travis due to the potential for long test output to a) kill jobs and b) be annoying to read.

[tfenne]

Ok, why don't I wait until the next release is cut, then rebase and merge this PR. If we run into problems with travis I'm sure we can find a not-so-elegant way to cut the verbosity of the tests.

[yfarjoun]

We just released a version. Feel free to rebase. we would also appreciate a first draft of guidelines as to what is an acceptable style of scala tests to use. Perhaps in a wiki page?

[tfenne]

@yfarjoun I've just rebased and squashed. Apologies for taking so long on that. I'd like to get this merged if at all possible. I'm still happy to write the wiki page, but I've sort of forgotten what it was supposed to address ;) Obviously some examples, anything else?

[yfarjoun]

👍 on the PR.

Regarding the wiki-page, some examples would be good as well as an explanation of the different kinds of comparisons that can be done (shouldBe vs should equal vs. ???)

The page would also lay out what is reasonable scala to use and what is out of scope. We would like to avoid having Haskel-like scala, and stick to java-like scala, so that fluency in scala is not a requirement for adding tests.

[tfenne]

@yfarjoun I've started a wiki page here: https://github.com/samtools/htsjdk/wiki/Writing-tests-in-HTSJDK . Please feel free to both edit it yourself, and suggest additions. It's a little hard for me having been working so much in scala over the last couple of years to think about what features are more or less readable to Java developers etc.

I'm going to click merge now!

[yfarjoun]

looks good thanks!

…

On Sat, Apr 15, 2017 at 2:00 PM, Tim Fennell ***@***.***> wrote: Merged #789 <#789>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#789 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACnk0kNTBMleXfkAPgrL9fXn4LODJOezks5rwQW9gaJpZM4Lp26L> .

[yfarjoun]

@tfenne I tried (and failed) to run the tests from within intelliJ. Are there special settings that need to be set? could you clarify what they would be?

[tfenne]

The only things I had to do were:

1. Re-import the build file in Intellij (in the gradle window hit refresh)
2. Select the right Scala SDK to use (2.12.x), which I was prompted to do on refresh

If that doesn't work, can you see what version of IntelliJ and the Scala plugin you're using?

[yfarjoun]

Thanks. it was the 2.12.x scala version (I had 2.10)