AWS: Delegate part of AWS integration tests to using mock aws services and enable tests in check task #12671
Conversation
… enable tests in check task
| @@ -290,6 +290,10 @@ public class S3FileIOProperties implements Serializable { | |||
|
|
|||
| public static final boolean CHECKSUM_ENABLED_DEFAULT = false; | |||
|
|
|||
| public static final String CHUNK_ENCODING_ENABLED = "s3.chunked-encoding.enabled"; | |||
|
|
|||
| public static final boolean CHUNK_ENCODING_ENABLED_DEFAULT = true; | |||
There was a problem hiding this comment.
Do we need to update doc for this?
There was a problem hiding this comment.
can you please elaborate why this is needed?
There was a problem hiding this comment.
I'm just thinking it's a new property and asking whether it should be documented.
There was a problem hiding this comment.
@nastra @manuzhang Thanks for thew review!
Yes, we should document this and I can make the change.
The reason to add CHUNK_ENCODING_ENABLED, is that I found this needs to be disabled when using moto with s3, otherwise there will be encoded chunk signatures appear in the file contents and causing parsing errors. I have not yet file an issue.
If this can be fixed, then the property used in the tests will not be needed.
S3FileIOProperties.CHUNK_ENCODING_ENABLED="false"
| @@ -56,8 +58,7 @@ | |||
| import software.amazon.awssdk.services.s3.model.HeadObjectRequest; | |||
| import software.amazon.awssdk.services.s3.model.NoSuchKeyException; | |||
|
|
|||
| public class TestDynamoDbCatalog { | |||
|
|
|||
| public class TestMockDynamoDbCatalog extends BaseAwsMockTest { | |||
There was a problem hiding this comment.
can we achieve the same without having to rename tests? I don't think it's clear that tests need to have a Mock in the name to properly be executed and it's very likely that this will be missed in the future if new tests are added. Hence I would suggest to keep the test names as they are
There was a problem hiding this comment.
Right now I use filtering to only run tests start with TestMock, but I agree it's not very clear. I think we could maybe introduce a custom annotation. Let me try out new approaches and give update on this.
There was a problem hiding this comment.
Seems like using Tag and
useJUnitPlatform {
includeTags 'verification'
}
is the cleanest approach
| assertThat(results).as("should have only 1 process succeeded in acquisition").hasSize(1); | ||
| assertThat(results) | ||
| .as("should have only 1 process succeeded in acquisition") | ||
| .hasSize(16) |
There was a problem hiding this comment.
the comment says 1 but this checks for 16
There was a problem hiding this comment.
The test has been failing before the change.
[should have only 1 process succeeded in acquisition]
Expected size: 1 but was: 16 in:
[false, true, false, false, ...]
Because 16 tasks are trying to acquire the lock and only 1 will be returning true others are returning false. We could also update the comment to be should have only 1 process succeeded in 16 parallel acquisitions
assertThat(results)
.as("should have only 1 process succeeded in acquisition")
.hasSize(16)
.containsOnce(true)
| MOTO_CONTAINER.start(); | ||
|
|
||
| // Set dummy AWS environment variables | ||
| System.setProperty("aws.accessKeyId", "testing"); |
There was a problem hiding this comment.
which part makes sure that those system properties are cleared again after testing? I believe those should be rather set when the client gets initialized rather than just generically setting these things via system properties
There was a problem hiding this comment.
I think the issue is that currently only s3 client support static credentials and is brought up in this #10614
as well. We could unset the property in afterAll.
Description
testtointegrationTestfolderverificationIntegrationTestthat will run selected integration tests that does not need AWS account / credentials.Fixes #12236