search

RusticiSoftware / TinCanJava

Tin Can Java Library
http://rusticisoftware.github.io/TinCanJava/
Apache License 2.0
44 stars 46 forks source link

Added ability to send and receive attachments #55

Closed FishHooks closed 8 years ago

FishHooks commented 8 years ago

Now with more rebase!

brianjmiller commented 8 years ago

I think the unit tests around this need to be beefed up a bit. The assertion level in the unit tests isn't great for the rest of the suite for the moment, but in this specific case I think we need to test the returned body of the attachments themselves match the sent content, possibly by hashing to make sure we don't run into encoding issues, etc. Have a look at the PHP lib's tests for examples, at the very least it would be good to run a binary file through and make sure it hashes the same and that the attachment hash itself matches what we expect.

brianjmiller commented 8 years ago

@FishHooks I added a binary file attachment test in my attachments branch of my fork which doesn't pass the round trip test. I'm still not convinced the serialization/deserialization is working correctly with respect to how strings are involved.

shasum -a 256 src/test/resources/files/image.jpg 2d4695073b46a0b392c288d65be6c32b78457fe82d39e71c02d47046cc15a921 src/test/resources/files/image.jpg

For what it should be, which matches what the LRS is storing (so send is good), but not what we get when calculating after retrieving the statement. To run the single test use:

mvn test -Dtest=RemoteLRSTest#testRetrieveStatementWithBinaryAttachment

You should be able to merge (fast forward) that commit into your branch and work from there.

FishHooks commented 8 years ago

@brianjmiller back to you. The binary and non-binary tests pass now