Repository navigation
Add S3 integration tests - #2067
PeterDowdy wants to merge 4 commits into
Conversation
… running these tests against minio and added a ci step Signed-off-by: Peter Dowdy <peter.dowdy@gmail.com> Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Peter Dowdy <peter.dowdy@gmail.com> Assisted-by: Claude:claude-sonnet-5
|
Thank you for this. I won't have time to look at it for the next few weeks but I will get round to it. |
No rush! |
|
Looking at this I have a some comments: Firstly (and most importantly) is looks like minio is not longer in development and has been archived. The tests get the latest docker image, if the project is no longer maintained it should probably get a specific image (just in case someone replaces the latest image with something nefarious). Secondly, the copyright mentions the Broad Institute. From your profile you do not appear to work for them so that needs changing. Thirdly (and this one is more subjective) your test file itself seems large and wordy. It is going to make it harder to check and maintain. All that being said we are thinking about using it for our maintenance checks (rather than the regular ones). |
|
Thanks for the feedback. I've addressed those issues. Especially re: the minio one, they stopped publishing the image between then and now, so I've pinned and switched providers. My agent noticed an edge-case bug in file reading that occurs when a file is exactly the size of a partition boundary. I wrote a test case to reproduce it and included it in this PR; do you want me to file a bug, include the fix here, or just drop it? |
Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Peter Dowdy <peter.dowdy@gmail.com>
Make it an issue please. |
The test documented that reading an S3 object whose size is an exact multiple of the read part size fails with EINVAL. It was an expected failure (F), so it did not exercise anything that works today. Drop it along with its aligned.tmp.bin setup until the underlying bug is fixed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011taCtDzqRYHL6y3P5sDtgy
3 participants
This PR adds S3 integration tests through minIO as a mocked S3 provider. It covers most of the happy and sad paths in the project, with moderate rigour. It doesn't check transient errors (since the library just dies on them anyways), TLS verification (since that seems like it's drifting out of testing S3), or very large synthetic files (to keep runtime short).
The minIO test depends on an env var so developers without minIO can safely bypass it.
This effort surfaced a few fairly small gaps in S3 file-handling that could be filled:
Assisted-by: Claude:claude-sonnet-5