Add integration tests for S3 Assume Role ingestion feature#11472
Add integration tests for S3 Assume Role ingestion feature#11472maytasm merged 5 commits intoapache:masterfrom
Conversation
| if (config.getS3AssumeRoleExternalId() == null || config.getS3AssumeRoleWithExternalId() == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Instead of exiting silently I think this should throw an exception. The caller should handle this exception gracefully and mark the test as skipped - instead of marking it as passed - since the test hasn't run yet.
Similar comments for the other functions in this class
There was a problem hiding this comment.
I have spent a few hours but was not able to achieve this. I have tried using Assume.assumeTrue which should throw an AssumptionViolatedException which the test runner should then handle gracefully and mark the test as skipped. However, the test runner doesn't and the AssumptionViolatedException caused the test to failed. I have tried looking at the version of junit, surefire, etc and was not able to figure out what's wrong. I also could not find any other way of programmatically skipping the test within the test code. Let me know if you have any idea of achieving this. (things like @Ignore does not work in this case as the skip have to be done programmatically within the test method).
There was a problem hiding this comment.
The integration tests are in testNG. That might be why the assume true statement didn't work.
I think you need to throw a skip exception to skip the tests. I found a few examples while googling but I haven't been able to verify any of these myself
There was a problem hiding this comment.
Oops you are right. Got the skip working. facepalm
| * should not have external id set) | ||
| * | ||
| * NOTE: Tests in this class will be skipped if properties in #4 are not set. | ||
| */ |
There was a problem hiding this comment.
🚀 these instructions are great! thank you!
suneet-s
left a comment
suneet-s
left a comment
There was a problem hiding this comment.
looks good but CI failure looks legit
3 participants
Add integration tests for S3 Assume Role ingestion feature
Description
This PR adds integration tests for S3 Assume Role ingestion feature introduced in https://github.com/apache/druid/pull/10995/files
The new integration tests are not run as part of Travis since it requires S3 credentials of an active AWS account. While the new integration tests are added to existing test group, they will be skip if the required properties for S3 Role are not provided (
-Ddruid.test.config.s3AssumeRoleWithExternalId,-Ddruid.test.config.s3AssumeRoleExternalId,-Ddruid.test.config.s3AssumeRoleWithoutExternalId). This is done in order to keep existing test groups passing without the immediate needs to provide the new properties.This PR has: