-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add a new "contains" feature to yml test #34738
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
Conversation
The contains syntax was added in elastic#30874 but the skips were not properly put in place. The java runner has the feature so the tests will run as part of the build, but language clients will be able to support it at their own pace.
|
Pinging @elastic/es-core-infra |
|
I used the following commands to make sure I get them all: |
javanna
left a comment
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 should we also enforce that tests which contain a contains assertion also need to have the skip for feature contains?
This has not been caught by clients as none of the affected tests is run by them, but supporting such assertion make it possible for us to use it anywhere so it is still a good idea to require the skip section.
| # | ||
| "Matrix stats aggs loaded": | ||
| - skip: | ||
| reason: "contains is a newly added assertion" |
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 believe the reason can be skipped when you specify a feature. it is required only when specifying versions
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.
@atorok ping not sure you have seen this comment
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.
Sorry @javanna I did not interpret that as something you would like me to change.
The reason is overly repetitive even if used with version, but it does seem to add some more context with someone not as familiar with this syntax. That's why I left it in.
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.
it's not a big deal, just that I think skip features contains is already self-explaining and adding a reason does not add value, but I am fine with keeping it. I did not mean that I wanted you to change it, I was ok with you replying that you are leaving it as-is, just so I know you've seen the comment ;)
tvernum
left a comment
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 👍
|
Thanks for the reviews. @javanna I'll implement your recommendations in a separate PR. |
The contains syntax was added in #30874 but the skips were not properly put in place. The java runner has the feature so the tests will run as part of the build, but language clients will be able to support it at their own pace.
The contains syntax was added in #30874 but the skips were not properly
put in place.
The java runner has the feature so the tests will run as part of the
build, but language clients will be able to support it at their own
pace.