Allow unzipping using an offset into the uncompressed and compressed …

[nh13]

@tfenne or @akiezun want to take a look at this? For tools that have read in one large byte array, it is more efficient to provide the whole array and offsets into the array, rather than copy subsets of the arrays and use the method without offsets.

[coveralls]

Coverage increased (+0.007%) to 68.419% when pulling 9719064 on nh_gunzipper into ec44a54 on master.

[akiezun]

Folks, for things like this we really need tests. The htsjdk test coverage is very bad as it is and for nontrivial changes we must make sure we're making the library more tested and more robust with every pull request. This library is a key piece of so many projects. I think we should vote on a policy regarding testing non trivial changes, @droazen @yfarjoun @lbergelson wdyt?

[tfenne]

Not that I disagree with the sentiment @akiezun but I actually reviewed the coveralls report being merging this and was satisfied that all modified code was covered by existing test cases.

[nh13]

I agree Adam that new functionality should be well tested, and where reasonable (in the eyes of the contributor), tests should be added when modifying existing code. At the moment, I would vote no to any policy as I am concerned we would do so just for the sake of a policy rather than use good judgement and contextual reasoning for what is needed for testing. I suppose if you really wanted a vote, you first have to identify those that get a vote and what their vote's relative contribution is.

[akiezun]

Thanks @nh13 and @tfenne. In this case, maybe it was a refactoring but the general point still stands. Quality of this library is important to all of us and our projects. I think we should aim to improve it with every code change and testing is paramount to quality (line coverage is really only a crutch we use to get at the actual coverage of the cases). Most code changes are of one of 3 kinds:
a) new functionality
b) bug fix
c) refactoring (no change in behavior - just a change in something else)

In a) tests need to be added to demonstrate that the new feature works properly and will continue to work properly
In b) test should be added to show that the bug exists before the fix and the bug is gone after the fix - so the tests should fail without the changes and pass with the changes.
Only in c) is it ok to not implement new tests.

[yfarjoun]

There's also
d) improvement in performance (which is what this is.)
Since we do not have a framework for performance testing in htsjdk, I'm not
sure what should be done in this case...

On Mon, Jun 27, 2016 at 11:48 AM, Adam Kiezun notifications@github.com
wrote:

Thanks @nh13 https://github.com/nh13 and @tfenne
https://github.com/tfenne. In this case, maybe it was a refactoring but
the general point still stands. Quality of this library is important to all
of us and our projects. I think we should aim to improve it with every code
change and testing is paramount to quality (line coverage is really only a
crutch we use to get at the actual coverage of the cases). Most code
changes are of one of 3 kinds:
a) new functionality
b) bug fix
c) refactoring (no change in behavior - just a change in something else)

In a) tests need to be added to demonstrate that the new feature works
properly and will continue to work properly
In b) test should be added to show that the bug exists before the fix and
the bug is gone after the fix - so the tests should fail without the
changes and pass with the changes.
Only in c) is it ok to not implement new tests.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#651 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACnk0h99TdmJS0zwk1W1iIBGukzVbLntks5qP_DKgaJpZM4I-RZj
.