Repository navigation
CRC64Checksum test #944 - #951
Conversation
sbliven
left a comment
There was a problem hiding this comment.
Thanks a lot for these tests! Improving our code coverage is great. The CRC64Checksum class seems to have been copied from expasy4j, so it
Regarding the partialbyteRange test, I'm not sure that this captures the intended behavior. Is the function used like this elsewhere? Unless there's a good reason to change it, I would prefer making this test pass now and then fixing any places that are expecting (bytes, start, length) at the caller.
| @Disabled | ||
| // this doesn't work as expected | ||
| // and doesn't behave according to interface. The 3rd argument | ||
| // is treated as an index rather than a number of bytes to include |
There was a problem hiding this comment.
It looks to me that the function is intended to take (bytes, start, end) rather than (bytes, start, length). I think this is a failure with the documentation (both non-existent javadoc and the misleadingly named parameters). It looks like it should run as expected with crc64.update(testBytes, 2, 3);.
There was a problem hiding this comment.
This method doesn't seem to be used anywhere internally in BioJava. The behaviour is specified in the interface java.util.zip.Checksum which this class implements (javadoc reproduced below). Since this is an interface in the standard Java library we can't change the contract of the interface. If we assume that there is some external client code using this method, then from their point of view fixing this might be a breaking change, but this would be OK if it's documented and is in version 6?
Updates the current checksum with the specified array of bytes.
*
* @param b the byte array to update the checksum with
* @param off the start offset of the data
* @param len the number of bytes to use for the update
There was a problem hiding this comment.
I think it'd be ok introducing the breaking change for 6.0.0. For sure we should document it in the CHANGELOG
There was a problem hiding this comment.
Pushed this fix and updated the changelog.
Also, added some input validation, following example of java.util.zip.CRC32 implementation
|
Hi. |
|
I think @sbliven would have to approve. In any case, I think his comments were addressed. Let's give it a couple of days and then merge. |
CRC64Checksum test biojava#944
Test for basic behaviour of Crc64Checksum - #944
I think there is a bug exposed by test
partialbyteRange()and I've proposed a solution in the Crc64Checksum class.