Increase code coverage on dynamodb-enhanced module#6700
Increase code coverage on dynamodb-enhanced module#6700andreas-grafenberger wants to merge 21 commits into
Conversation
a024740 to
fcfbd00
Compare
337c117 to
f97adfc
Compare
1764264 to
8dd1add
Compare
5016df9 to
e33be5f
Compare
…dding assert for OptionalAttributeValueConverterTest.
…as part of PR 6373
… 437-increase_test_coverage # Conflicts: # services-custom/dynamodb-enhanced/src/test/java/software/amazon/awssdk/enhanced/dynamodb/functionaltests/extensions/AutoGeneratedUuidExtensionTest.java
# Conflicts: # services-custom/dynamodb-enhanced/src/test/java/software/amazon/awssdk/enhanced/dynamodb/internal/operations/DeleteTableOperationTest.java
# Conflicts: # services-custom/dynamodb-enhanced/src/test/java/software/amazon/awssdk/enhanced/dynamodb/internal/EnhancedClientUtilsTest.java
| import org.apache.logging.log4j.core.config.Configuration; | ||
| import org.apache.logging.log4j.core.config.LoggerConfig; | ||
|
|
||
| public final class LogCaptor implements AutoCloseable { |
There was a problem hiding this comment.
Any reason we can't use the existing LogCaptor class? https://github.com/aws/aws-sdk-java-v2/blob/master/test/test-utils/src/main/java/software/amazon/awssdk/testutils/LogCaptor.java
There was a problem hiding this comment.
LogCaptor you mention is part of test-utils module, but that module was not added as dependency for dynamodb-enhanced module. Given this small functionality that was needed, I prefered to create a single-class (LogCaptor) instead of importing the entire module (test-utils).
Also, the functionality of the new LogCaptor is manipulating only the targeted logger configuration (adding a new appender and changing the logging level), and doesn't touch the root logger / other loggers configuration (as .../test-utils/.../LogCaptor does).
Another reason is - in the context of .../test-utils/.../LogCaptor, if someone sets any additivity property on false for any monitorized logger, then the tests would fail because the events are not propagated anymore to the root logger (from my point of view, this is a buggy implementation which doesn't allow proper control over logging framework).
To reproduce this buddy behavior - e.g:
Step 1) Go to pom.xml and add dependency to test-utils:
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>test-utils</artifactId>
<scope>test</scope>
</dependency>Step 2) Go to class: DefaultAttributeConverterProviderTest.class
and replace:
try (LogCaptor logCaptor = new LogCaptor(DefaultAttributeConverterProvider.class, Level.DEBUG)) {with:
try (software.amazon.awssdk.testutils.LogCaptor logCaptor =
software.amazon.awssdk.testutils.LogCaptor.create(org.apache.logging.log4j.Level.DEBUG)) {Step 3) Go to services-custom/dynamodb-enhanced/src/test/resources/log4j2.properties and use a custom logger configuration:
logger.defaultAttributeConverterProvider.name = software.amazon.awssdk.enhanced.dynamodb.DefaultAttributeConverterProvider
logger.defaultAttributeConverterProvider.level = warn
logger.defaultAttributeConverterProvider.additivity = false
logger.defaultAttributeConverterProvider.appenderRef.console.ref = ConsoleAppender
| .stream() | ||
| .flatMap(this::createTestsForInterface) | ||
| .collect(toList()); | ||
| assertEquals(102, dynamicTestList.size()); |
There was a problem hiding this comment.
This seems brittle; any future interface change breaks this with a confusing failure. Consider "assertThat(...).isNotEmpty()" instead
There was a problem hiding this comment.
Agree - this was the purpose, to gain more control over methods that throw UnsupportedOperation and avoid mistakes. Replacing that assertion with assertThat(...).isNotEmpty() makes the test lose its value.
| import software.amazon.awssdk.services.dynamodb.model.TableDescription; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class AnnotatedTableSchemaTest extends LocalDynamoDbSyncTestBase { |
There was a problem hiding this comment.
Looks like there is a lot of duplication between AnnotatedTableSchemaTest (715 lines) and AsyncAnnotatedTableSchemaTest (726 lines). At minimum, can we extract the factory to a shared class?
There was a problem hiding this comment.
I did so, please see commit: #386e270, modified classes:
- .../enhanced/dynamodb/functionaltests/AnnotatedTableSchemaTest.java
- .../enhanced/dynamodb/functionaltests/AsyncAnnotatedTableSchemaTest.java
and added class:
| import software.amazon.awssdk.services.dynamodb.model.ConsumedCapacity; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) | ||
| public class EnhancedClientUtilsTest { |
There was a problem hiding this comment.
The file contains duplicate test pairs:
readAndTransformSingleItem_withNullItemMap_returnsNullandreadAndTransformSingleItem_nullItemMap_returnsNull— same test, different namesreadAndTransformSingleItem_withEmptyItemMap_returnsNullandreadAndTransformSingleItem_emptyItemMap_returnsNull— same test, different namescreateKeyFromItem_withPartitionKeyOnly_createsCorrectKeyandcreateKeyFromItem_itemWithPartitionKeyOnly_returnsKeyWithoutSortKey— same test, different na
mes
There was a problem hiding this comment.
I missed that - the cleanup was made in commit: #e547100, thanks!
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; |
There was a problem hiding this comment.
Junit5 is preferred for new test classes https://github.com/aws/aws-sdk-java-v2/blob/master/docs/guidelines/testing-guidelines.md, but I see we are extending existing class that already uses Junit4, so this is probably fine
There was a problem hiding this comment.
Yes, I would keep it as it is.
The reason I used Junit4 is related to the class parameterization (instead of method parameterization from Junit 5).
As you already noticed, the base class LocalDynamoDbTestBase already uses Junit4 and mixing both versions of Junit (4 and 5) is causing troubles in some cases, just an example: when using after / before behavior.
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.is; | ||
| import static org.hamcrest.Matchers.nullValue; |
There was a problem hiding this comment.
assertJ is preferred over hamcrest for new tests https://github.com/aws/aws-sdk-java-v2/blob/master/docs/guidelines/testing-guidelines.md
There was a problem hiding this comment.
I performed a quick check through all modified classes and addressed this suggestion - see commit: 87a6083. The commit contains only this kind of refactoring (including for existing tests from modified classes).
| public void builder_startAtValueIsLessThanMinusOne_throwsIllegalArgumentException() { | ||
| assertThrows(IllegalArgumentException.class, | ||
| () -> VersionedRecordExtension.builder().startAt(-2L).build(), | ||
| "startAt must be -1 or greater"); | ||
| } | ||
|
|
||
| @Test | ||
| public void builder_incrementByValueIsLessThanOne_throwsIllegalArgumentException() { | ||
| assertThrows(IllegalArgumentException.class, | ||
| () -> VersionedRecordExtension.builder().incrementBy(0L).build(), | ||
| "incrementBy must be greater than 0."); |
There was a problem hiding this comment.
Can we use parameterized tests?
There was a problem hiding this comment.
I did so, please see commit: #386e270, modified class:
| import software.amazon.awssdk.enhanced.dynamodb.mapper.annotations.DynamoDbPartitionKey; | ||
| import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; | ||
|
|
||
| public class VersionedRecordExtensionTest extends LocalDynamoDbSyncTestBase { |
There was a problem hiding this comment.
Can we use ParameterizedTest for versionAttribute_withInvalid* tests?
There was a problem hiding this comment.
I did so, please see commit: #386e270, modified class:
| } | ||
|
|
||
| @Test | ||
| public void generateRequest_mergesKeysAndAttributes_incompatibleConsistentRead_throws() { |
There was a problem hiding this comment.
Same here, it'd be great if we can use ParameterizedTests
There was a problem hiding this comment.
I implemented some parameterization, please see commit: #386e270, modified class:
|
Closing in favor of #6969 |
Motivation and Context
The goal of this work is to increase unit, functional, and integration code coverage for the
aws-sdk-java-v2/services-custom/dynamodb-enhancedmodule.Initial coverage:
Target coverage:
Achieved coverage:
These results exceed the defined targets and significantly improve confidence in the correctness and stability of the enhanced DynamoDB module.
Modifications
This pull request expands code coverage across the DynamoDB Enhanced Client by introducing new unit, functional, and integration tests targeting previously uncovered or partially covered code paths.
Testing
Screenshots
Initial coverage:
Achieved coverage:
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License