-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Differentiate base paths in repository integration tests #47284
Conversation
Pinging @elastic/es-distributed |
@@ -88,7 +88,7 @@ task thirdPartyTest(type: Test) { | |||
systemProperty 'test.azure.key', azureKey ? azureKey : "" | |||
systemProperty 'test.azure.sas_token', azureSasToken ? azureSasToken : "" | |||
systemProperty 'test.azure.container', azureContainer ? azureContainer : "" | |||
systemProperty 'test.azure.base', azureBasePath ? azureBasePath : "" | |||
systemProperty 'test.azure.base', (azureBasePath ? azureBasePath : "") + "_third_party_tests_" + project.testSeed |
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.
@tlrx thanks for this one!
I think technically this is the perfect fix in terms of stability. One problem we're creating here though is that we are leaking a bunch of blobs for each test run now.
Should we add some logic to delete the leaked folders maybe? (otherwise we force infra to deal with this somehow which will be tricky for them I think)
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.
To avoid this issue for now: Maybe we can simply add a constant path here and another constant in the other build so we don't get interference between the test runs? We don't have to worry about different builds interfering with each other, infra makes sure that doesn't happen so that should be fine?
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.
Yeah, I wrongly assumed that the container/buckets were recreated on demand by some Infra task before running the tests.
I think we can easily keep the base_path with test seed for the 3rd party tests, as we can easily clean up the files when tearing down the test (similarly to what the test set up is already doing doing)
For the repository integration test, I'll go with adding a constant.
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.
LGTM (assuming CI is happy too) :) Its retriever the 3rd party tests right away after merging this to make sure we're good here.
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.
Seems CI is unhappy, that teardown doesn't work with HDFS repo for some reason.
@elasticmachine update branch |
@original-brownbear I've updated to code to fix the HDFS test issue. I've also added changes for S3/GCS (will update title+desc when merging) alongside. Can you please have another look? |
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.
LGTM (if CI loves it :D), thanks @tlrx !
Thanks Armin! |
This just failed on 6.8 as well, can it be backported there? |
Thanks @romseygeek - I opened #48286 |
Abstract third party tests do not exist on 6.8 but we can still differentiate base paths in repository integration tests like we did for other branches. Relates #47284
This commit change the repositories base paths used in Azure integration tests so that they don't conflict with each other when tests run in parallel on real Azure storage service.
Closes #47202