test: implement integration tests for S3 FileService#49
Merged
Conversation
📝 WalkthroughWalkthroughAdds a JUnit 5 integration test class Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java (3)
42-42: Typo in test method name:ShouldReturnSKey→ShouldReturnS3Key.Minor naming polish for readability in test reports.
✏️ Proposed rename
- void uploadFile_ShouldReturnSKey_WhenUploadIsSuccessful() throws IOException { + void uploadFile_ShouldReturnS3Key_WhenUploadIsSuccessful() throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` at line 42, Rename the test method uploadFile_ShouldReturnSKey_WhenUploadIsSuccessful() to uploadFile_ShouldReturnS3Key_WhenUploadIsSuccessful() to fix the typo; update the method declaration and any references to that method (e.g., in test suites or IDE run configurations) so the test name reads "S3Key" for clarity in test reports.
9-17: Redundant/overlapping imports.Line 15 uses a wildcard
software.amazon.awssdk.services.s3.model.*which already coversPutObjectRequestandPutObjectResponseexplicitly imported on lines 11–12. Either drop the wildcard and keep explicit imports (preferred) or remove the now-redundant explicit ones. Also, the twoorg.assertj.core.api.Assertionsstatic imports on lines 14 and 23 could be consolidated into a single block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 9 - 17, The test file has overlapping imports: remove the broad wildcard import software.amazon.awssdk.services.s3.model.* and keep the explicit imports like PutObjectRequest and PutObjectResponse (and other needed model types) so imports are explicit; also consolidate duplicate static imports of org.assertj.core.api.Assertions.assertThat into a single static import statement to avoid redundancy and improve clarity—update the import block to only include S3Client, S3Presigner, the explicit model classes you use (e.g., PutObjectRequest, PutObjectResponse, GetObjectRequest, etc.), and a single static import for Assertions.assertThat.
142-154: Add a test for error handling indeleteFile.The current test covers the happy path. Since
deleteFiledoes not explicitly handle exceptions, adding a test that stubss3Client.deleteObject(...)to throw an exception would document the error-handling contract and ensure the method propagates exceptions as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 142 - 154, Add a test in FileServiceIntegrationTest that verifies deleteFile propagates exceptions by stubbing s3Client.deleteObject(...) to throw (use Mockito doThrow/when) and asserting that calling fileService.deleteFile(s3Key) results in an exception (use assertThrows), referencing the deleteFile method, s3Client.deleteObject and DeleteObjectRequest to locate the call and ensure the test verifies propagation rather than swallowing the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java`:
- Line 69: Rename the test method
uploadFile_shouldThrowException_WhenFileIsToLarge to correct the typo and match
sibling test casing: change it to
uploadFile_ShouldThrowException_WhenFileIsTooLarge; update any references (test
calls, IDE run configs) to the old name so the test runner and imports remain
consistent.
---
Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java`:
- Line 42: Rename the test method
uploadFile_ShouldReturnSKey_WhenUploadIsSuccessful() to
uploadFile_ShouldReturnS3Key_WhenUploadIsSuccessful() to fix the typo; update
the method declaration and any references to that method (e.g., in test suites
or IDE run configurations) so the test name reads "S3Key" for clarity in test
reports.
- Around line 9-17: The test file has overlapping imports: remove the broad
wildcard import software.amazon.awssdk.services.s3.model.* and keep the explicit
imports like PutObjectRequest and PutObjectResponse (and other needed model
types) so imports are explicit; also consolidate duplicate static imports of
org.assertj.core.api.Assertions.assertThat into a single static import statement
to avoid redundancy and improve clarity—update the import block to only include
S3Client, S3Presigner, the explicit model classes you use (e.g.,
PutObjectRequest, PutObjectResponse, GetObjectRequest, etc.), and a single
static import for Assertions.assertThat.
- Around line 142-154: Add a test in FileServiceIntegrationTest that verifies
deleteFile propagates exceptions by stubbing s3Client.deleteObject(...) to throw
(use Mockito doThrow/when) and asserting that calling
fileService.deleteFile(s3Key) results in an exception (use assertThrows),
referencing the deleteFile method, s3Client.deleteObject and DeleteObjectRequest
to locate the call and ensure the test verifies propagation rather than
swallowing the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4de79ad9-ac44-429d-8623-a2567277f9ee
📒 Files selected for processing (1)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java
| } | ||
|
|
||
| @Test | ||
| void uploadFile_shouldThrowException_WhenFileIsToLarge() { |
There was a problem hiding this comment.
Typo: WhenFileIsToLarge → WhenFileIsTooLarge.
✏️ Proposed rename
- void uploadFile_shouldThrowException_WhenFileIsToLarge() {
+ void uploadFile_ShouldThrowException_WhenFileIsTooLarge() {Also note the lowercase shouldThrowException here is inconsistent with the Pascal-case ShouldThrowException used in the sibling test on line 105.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void uploadFile_shouldThrowException_WhenFileIsToLarge() { | |
| void uploadFile_ShouldThrowException_WhenFileIsTooLarge() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java`
at line 69, Rename the test method
uploadFile_shouldThrowException_WhenFileIsToLarge to correct the typo and match
sibling test casing: change it to
uploadFile_ShouldThrowException_WhenFileIsTooLarge; update any references (test
calls, IDE run configs) to the old name so the test runner and imports remain
consistent.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java (3)
77-79: Assert validation failures do not attempt an S3 upload.The exception assertions are good, but these tests would also pass if validation happened after an attempted
putObjectside effect. Add a negative verification for the upload call.Suggested negative verifications
assertThatThrownBy(() -> fileService.uploadFile(largeFile)) .isInstanceOf(IOException.class) .hasMessageContaining("File exceeds maximum allowed size of 10 MB"); + verify(s3Client, never()).putObject(any(PutObjectRequest.class), any(RequestBody.class));assertThatThrownBy(() -> fileService.uploadFile(mockFile)) .isInstanceOf(IOException.class) .hasMessageContaining("Unsupported document type: " + mockFile.getContentType()); + verify(s3Client, never()).putObject(any(PutObjectRequest.class), any(RequestBody.class));Also applies to: 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 77 - 79, The tests currently assert that fileService.uploadFile(largeFile) throws an IOException but don’t assert that no S3 upload was attempted; after each assertThatThrownBy add a negative verification that the S3 client’s upload method was not invoked (e.g. verify(s3Client, never()).putObject(...)) to ensure validation fails before any call to the S3 upload; apply the same change to the second failing test around lines 115-117 as well so both tests verify no putObject/upload call was made.
132-139: Avoidany()for the presign request.This test currently only proves that
url()is returned from the mocked presigner. It would still pass ifFileServicebuilt the presign request with the wrong key, bucket, response disposition, or duration.Suggested request matcher
- when(s3Presigner.presignGetObject(any(GetObjectPresignRequest.class))) + when(s3Presigner.presignGetObject(argThat((GetObjectPresignRequest req) -> + req.signatureDuration().equals(java.time.Duration.ofMinutes(10)) + && "test-bucket".equals(req.getObjectRequest().bucket()) + && fileName.equals(req.getObjectRequest().key()) + && ("attachment; filename=\"" + fileName + "\"") + .equals(req.getObjectRequest().responseContentDisposition())))) .thenReturn(mockPresignedRequest);Please verify the exact accessor names against the AWS SDK v2 version used by the project if applying this matcher.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 132 - 139, The test uses any(GetObjectPresignRequest.class) when stubbing s3Presigner.presignGetObject, so it doesn’t verify that FileService.getPresignedDownloadUrl builds the presign request with the correct bucket, key, content-disposition and duration; replace the any() stub with a Mockito argument matcher (e.g., argThat) that inspects the GetObjectPresignRequest passed into s3Presigner.presignGetObject and asserts the expected values (check request.bucket(), request.key(), request.getObjectRequest().responseContentDisposition() or equivalent accessor for content-disposition, and the request signature/duration accessor used by the AWS SDK v2) before returning mockPresignedRequest so the test fails if FileService constructs the wrong request.
51-65: Verify the uploaded key and content type, not just the bucket.This test would still pass if
uploadFileuploaded a different key than the one it returns, or omitted the request content type. Tighten the matcher to cover the S3 request contract.Suggested assertion hardening
verify(s3Client).putObject( - argThat((PutObjectRequest req) -> req.bucket().equals("test-bucket")), + argThat((PutObjectRequest req) -> + "test-bucket".equals(req.bucket()) + && resultKey.equals(req.key()) + && "application/pdf".equals(req.contentType())), any(RequestBody.class) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 51 - 65, The test currently only verifies S3 bucket and returned filename; tighten it by asserting the actual PutObjectRequest contains the expected object key and content type so uploadFile cannot return a different key than it uploaded or omit content type. After calling fileService.uploadFile(mockFile), capture the expected key (the returned resultKey) and update the verify on s3Client.putObject to argThat a PutObjectRequest whose key() equals that expected key and whose contentType() (or contentType header/metadata) equals "application/pdf" (or the MIME your service sets), and keep the RequestBody matcher as any(RequestBody.class). Also keep the existing assertions on resultKey format to ensure both the return value and the actual S3 request are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java`:
- Around line 77-79: The tests currently assert that
fileService.uploadFile(largeFile) throws an IOException but don’t assert that no
S3 upload was attempted; after each assertThatThrownBy add a negative
verification that the S3 client’s upload method was not invoked (e.g.
verify(s3Client, never()).putObject(...)) to ensure validation fails before any
call to the S3 upload; apply the same change to the second failing test around
lines 115-117 as well so both tests verify no putObject/upload call was made.
- Around line 132-139: The test uses any(GetObjectPresignRequest.class) when
stubbing s3Presigner.presignGetObject, so it doesn’t verify that
FileService.getPresignedDownloadUrl builds the presign request with the correct
bucket, key, content-disposition and duration; replace the any() stub with a
Mockito argument matcher (e.g., argThat) that inspects the
GetObjectPresignRequest passed into s3Presigner.presignGetObject and asserts the
expected values (check request.bucket(), request.key(),
request.getObjectRequest().responseContentDisposition() or equivalent accessor
for content-disposition, and the request signature/duration accessor used by the
AWS SDK v2) before returning mockPresignedRequest so the test fails if
FileService constructs the wrong request.
- Around line 51-65: The test currently only verifies S3 bucket and returned
filename; tighten it by asserting the actual PutObjectRequest contains the
expected object key and content type so uploadFile cannot return a different key
than it uploaded or omit content type. After calling
fileService.uploadFile(mockFile), capture the expected key (the returned
resultKey) and update the verify on s3Client.putObject to argThat a
PutObjectRequest whose key() equals that expected key and whose contentType()
(or contentType header/metadata) equals "application/pdf" (or the MIME your
service sets), and keep the RequestBody matcher as any(RequestBody.class). Also
keep the existing assertions on resultKey format to ensure both the return value
and the actual S3 request are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90afbc76-0567-4e52-8a4a-0440bc8247ca
📒 Files selected for processing (1)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java (1)
11-17: Redundant wildcard import overlaps with explicit imports.Line 15 imports
software.amazon.awssdk.services.s3.model.*, which already coversPutObjectRequest(line 11) andPutObjectResponse(line 12). Collapse to a single form to avoid noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java` around lines 11 - 17, The import block contains a redundant wildcard import `software.amazon.awssdk.services.s3.model.*` which overlaps explicit imports like PutObjectRequest and PutObjectResponse; remove the wildcard and keep the explicit imports (PutObjectRequest, PutObjectResponse, S3Presigner, GetObjectPresignRequest, PresignedGetObjectRequest and any other used symbols such as assertThat) so only the exact required types are imported in FileServiceIntegrationTest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java`:
- Around line 11-17: The import block contains a redundant wildcard import
`software.amazon.awssdk.services.s3.model.*` which overlaps explicit imports
like PutObjectRequest and PutObjectResponse; remove the wildcard and keep the
explicit imports (PutObjectRequest, PutObjectResponse, S3Presigner,
GetObjectPresignRequest, PresignedGetObjectRequest and any other used symbols
such as assertThat) so only the exact required types are imported in
FileServiceIntegrationTest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17c55d76-aaf9-4ca1-a0c7-84fdb4b7597c
📒 Files selected for processing (1)
src/test/java/org/example/visacasemanagementsystem/file/FileServiceIntegrationTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit