-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRC64Checksum test #944 #951
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed this fix and updated the changelog.
Also, added some input validation, following example of java.util.zip.CRC32
implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
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. |
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.