-
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
[Rest Api Compatibility] Move won't fix tests from block list #73912
[Rest Api Compatibility] Move won't fix tests from block list #73912
Conversation
Pinging @elastic/es-delivery (Team:Delivery) |
my groovy is not best :D let me know how to improve this |
'cat.templates/10_basic/Normal templates', | ||
'cat.templates/10_basic/Select columns', | ||
'cat.thread_pool/10_basic/Test cat thread_pool output', | ||
// type information about the type is removed and not passed down. The logic to check for this is also removed. |
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.
Should we cleanup/remove some of these comments then given we don't plan on addressing any of these?
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.
I think those are beneficial. It is good to know the reason why the test is disabled without actually reading the test. The reasoning why it is disabled is clear now, but after a month or two (or earlier :D ) we might forget the details
@elasticmachine run Check labels |
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.
👍
rest-api-spec/build.gradle
Outdated
@@ -28,100 +28,102 @@ testClusters.all { | |||
|
|||
tasks.named("test").configure { enabled = false } | |||
tasks.named("jarHell").configure { enabled = false } | |||
|
|||
def willNotFixTests = { | |||
return ['cat.aliases/10_basic/Alias against closed index', |
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.
I think we can use wildcards in black lists. I would suggest to wildcard as much of these cat tests as possible to help with brevity and future cases. Also a brief mention that compatibility is not supported for cat APIs would help
rest-api-spec/build.gradle
Outdated
@@ -28,100 +28,102 @@ testClusters.all { | |||
|
|||
tasks.named("test").configure { enabled = false } | |||
tasks.named("jarHell").configure { enabled = false } | |||
|
|||
def willNotFixTests = { |
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.
nit: s/willNotFixTests/v7compatiblityNotSupportedTests
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
v7compatibilityNotSupportedTests was introduced to make it easier to track tests that have been identified as not needing compatible changes and those that still need to be checked. We have checked all tests now and the separate list is no longer needed. relates elastic#51816 relates elastic#73912
block list at the moment contains a lot of tests that we already identified that won't be fixed.
This commit moves them out of the main block list so that it is easier to count progress.