-
Notifications
You must be signed in to change notification settings - Fork 92
feat: replace external dependencies with mocks in unit tests #1946
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
base: main
Are you sure you want to change the base?
feat: replace external dependencies with mocks in unit tests #1946
Conversation
- Remove DynamoDB Local dependency from unit tests - Replace integration tests with proper unit tests using Mockito - Add E2E test scenario for real AWS infrastructure validation - Improve test performance and reliability by eliminating external dependencies - Add mockito-core dependency to powertools-idempotency-dynamodb module
Hi @Gdm0714, thank you very much for sending this PR. I will review it next week in more detail. In the meantime, can you have a look at the 7 new Sonarlint issues? https://sonarcloud.io/project/issues?id=aws-powertools_powertools-lambda-java&pullRequest=1946&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true
One question as well:
|
Hi! Thank you for the feedback and for taking the time to review this PR. I'll address the SonarLint issues you mentioned:
The main goal of this PR was to replace external dependencies (DynamoDB Local) with mocks in unit tests, not to add more E2E coverage. I can remove this additional E2E test method to keep the scope focused. Let me push the fixes for
Thanks again for the review! |
- Remove public modifiers from test classes and methods (JUnit5 package-private scope) - Remove unused imports (ResourceNotFoundException, Collections) - Remove unnecessary E2E test method for better scope focus
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.
Can this file be deleted now?
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.
Thank you for the feedback! I've deleted the DynamoDBConfig.java file as it was only used for DynamoDB Local server setup, which is no longer needed with our new mock-based approach.
|
||
@BeforeEach | ||
void setup() { | ||
MockitoAnnotations.openMocks(this); |
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.
Let's migrate the Mocking to using the Mockito extension for junit so that the mocks are opened automatically. See example here: https://github.com/aws-powertools/powertools-lambda-java/blob/main/powertools-kafka/src/test/java/software/amazon/lambda/powertools/kafka/PowertoolsSerializerTest.java#L49-L52
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.
Thank you for the suggestion about using MockitoExtension!
I've migrated both test classes to use @ExtendWith(MockitoExtension.class)
instead of the manual
MockitoAnnotations.openMocks()
approach. This provides better JUnit 5 integration and cleaner test setup.
Hey @Gdm0714, I reviewed your PR in more detail now. It looks like you removed a whole lot of unit tests and the assertions added might pass unit tests but do not actually assert correct code behavior. It is hard for me to understand the reasoning behind these changes. If you would like to proceed with your contribution I am happy to give you some more guidance on the quality bar for unit tests. I am happy to join a quick call with you to discuss your changes and the feedback in more detail. Please let me know if you are interested. Thanks! |
Hi! Thank you for the detailed feedback and for taking the time to review this thoroughly. You're absolutely right about my changes - I can see now that I oversimplified the unit tests and lost important business logic assertions in the process. I was too focused on removing the external dependency without preserving the actual test coverage and behavior validation. I would really appreciate the opportunity to discuss this with you in more detail. Thank you for your patience and guidance! |
- Add mockito-junit-jupiter dependency for MockitoExtension support
This file contained DynamoDB Local server setup which is not needed for self-contained unit tests with mocks.
- Replace MockitoAnnotations.openMocks() with @ExtendWith(MockitoExtension.class) - Remove public modifiers from test classes and methods (package-private) - Clean up unnecessary imports and mock stubbings - Ensure proper JUnit 5 + Mockito integration
0cfddd5
to
eb19c27
Compare
|
Summary
Changes
Issue number: #1932
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.