Add a new "contains" feature to yml test#34738
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.
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.
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.
@atorok ping not sure you have seen this comment
There was a problem hiding this comment.
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.
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 ;)
|
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.