Feature/dynamodb enhanced increase functional tests coverage - #6946
Feature/dynamodb enhanced increase functional tests coverage#6946iulianbudau wants to merge 1 commit into
Conversation
…verage refactoring Added changelog file for "Increase functional tests coverage on dynamodb-enhanced module" removed fully qualified references
edd446a to
08b4044
Compare
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "type": "feature", | |||
There was a problem hiding this comment.
Nit: I'd argue this should be a bugfix rather than a feature - I notice there are more than just test changes in the PR - we're including additional bugfixes in the AutoGeneratedTimestampRecordExtensionnced - that should be mentioned in the description as well.
| AttributeConverter<?> converter; | ||
| try { | ||
| converter = nestedSchema.converterForAttribute(key); | ||
| } catch (UnsupportedOperationException e) { |
There was a problem hiding this comment.
Do the test cases actually cover this? When I removed this check, all the tests still passed which implies we don't actually have sufficient coverage of a nested schema case that hits this.
| @Test | ||
| public void queryRecords_withEmptyProjectionString_shouldThrowAssertionError() { | ||
| insertNestedRecords(); | ||
| assertThatExceptionOfType(AssertionError.class).isThrownBy( |
There was a problem hiding this comment.
I think this test could pass for the wrong reason since its just looking for assertion failures in drain publisher.
I think we can change this to use the existing drainPublisherToError and then look for CompletionException. (and update the test name)
Note - I think this comment applies to a few tests:
- AsyncBasicQueryTest.queryRecords_withEmptyProjectionString_shouldThrowAssertionError
- AsyncBasicQueryTest.queryRecords_withEmptyNestedProjectionName_shouldThrowAssertionError
- AsyncBasicScanTest.scanRecords_withEmptyNestedAttributeName_shouldThrowAssertionError
- AsyncBasicScanTest.scanRecords_withEmptyTopLevelAttributeName_shouldThrowAssertionError
| assertThat(persisted.getDefaultCounter()).isEqualTo(2L); | ||
| assertThat(persisted.getCustomCounter()).isEqualTo(20L); | ||
| assertThat(persisted.getDecreasingCounter()).isEqualTo(-22L); |
There was a problem hiding this comment.
These feel like magic numbers. Where do they come from? We should at least comment how they were derived so that when they fail in the future we can understand why
| assertThat(persisted.getDefaultCounter()).isEqualTo(1L); | ||
| assertThat(persisted.getCustomCounter()).isEqualTo(15L); | ||
| assertThat(persisted.getDecreasingCounter()).isEqualTo(-21L); |
There was a problem hiding this comment.
These feel like magic numbers. Where do they come from? We should at least comment how they were derived so that when they fail in the future we can understand why
| @@ -0,0 +1,46 @@ | |||
| package software.amazon.awssdk.enhanced.dynamodb.functionaltests; | |||
There was a problem hiding this comment.
Nit: This file, along with other new ones are missing the copyright header.
| .setter(Record2::setAttribute)) | ||
| .build(); | ||
|
|
||
| private static final TableSchema<TimestampedRecord> TIMESTAMPED_TABLE_SCHEMA = |
There was a problem hiding this comment.
It looks like the TimestampedRecord and this TIMESTAMPED_TABLE_SCHEMA table schema are repeated in a number of places - should probably be extracted
| String extensionTableName = getConcreteTableName("doc-extension-table"); | ||
| DynamoDbEnhancedClient extensionClient = DynamoDbEnhancedClient.builder() | ||
| .dynamoDbClient(getDynamoDbClient()) | ||
| .extensions(AutoGeneratedTimestampRecordExtension.create()) | ||
| .build(); | ||
| DynamoDbTable<EnhancedDocument> extensionTable = | ||
| extensionClient.table(extensionTableName, | ||
| TableSchema.documentSchemaBuilder() | ||
| .addIndexPartitionKey(TableMetadata.primaryIndexName(), "id", AttributeValueType.S) | ||
| .attributeConverterProviders(defaultProvider()) | ||
| .build()); | ||
| extensionTable.createTable(r -> r.provisionedThroughput(getDefaultProvisionedThroughput())); |
There was a problem hiding this comment.
This looks like a lot of duplicated code between the new methods
| } | ||
|
|
||
| @Test | ||
| public void documentSchema_withExtension_putAndUpdate_shouldSucceed() { |
There was a problem hiding this comment.
It looks like these tests do not depend on the Paramaterized testData at all - which I believe means they will get run 10x+ times with identical data (and report the same success/failure many times). Assuming that is true, these should be moved out to a non parameterized test class.
I think this applies for the BasicAsyncCrudTest as well.
3 participants
Motivation and Context
The goal of this work is to increase functional tests coverage of the
aws-sdk-java-v2/services-custom/dynamodb-enhancedmodule through cross configuration (e.g. mixed schema types, shared client behavior, mixed extensions interaction) scenarios to prevent regressions for future changes.Testing
Expanded functional tests with scenarios on the below cross configurations:
Screenshots (if appropriate)
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