[TEST] Enforce skip headers when needed #34735
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The java yaml test runner supports sending request headers, yet not all clients support headers. This commit makes sure that we enforce adding a skip section with feature "headers" whenever headers are used in a do section as part of a test. That decreases the chance for new tests to break client builds due to the missing skip section. Closes elastic#34650
javanna
added
>enhancement
:Delivery/Build
|
Pinging @elastic/es-core-infra |
alpar-t
approved these changes
Oct 23, 2018
| if (false == doSection.getApiCallSection().getHeaders().isEmpty() | ||
| && false == skipSection.getFeatures().contains("headers")) { | ||
| throw new IllegalArgumentException("Attempted to add a [do] with a [headers] section without a corresponding " | ||
| + "[skip] so runners that do not support the [headers] section can skip the test at line [" |
There was a problem hiding this comment.
Maybe worth mentioning the feature has the same name to save people some digging ?
Suggested change
| + "[skip] so runners that do not support the [headers] section can skip the test at line [" | |
| + "[skip: {"feature": "headers"}] so runners that do not support the [headers] section can skip the test at line [" |
|
retest this please |
nik9000
approved these changes
Oct 24, 2018
| } | ||
| if (false == doSection.getApiCallSection().getHeaders().isEmpty() | ||
| && false == skipSection.getFeatures().contains("headers")) { | ||
| throw new IllegalArgumentException("Attempted to add a [do] with a [headers] section without a corresponding " |
|
@alpar or @nik9000 can you maybe have another look? I had to move the validation code higher-up, as this change made another bug surface: we don't look at the skip section from setup or teardown in our validation, and we have quite some skip headers in our setup sections, so false positives without adapting the validation code. Let me know what you think. |
nik9000
approved these changes
Oct 25, 2018
|
retest this please |
We now have more accurate validation of setup, teardown and test sections as for missing skip feature sections
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Oct 29, 2018
Validation of test section and suites consists of checking that the proper skip features sections are in place depending on the features used in tests. The validation logic was previously only performed on do sections included in each test section, and the skip needed to be present in the same test section. What happens often though is that the skip is added to the setup section, or the teardown section. This commit improves the validation of test suites by validating setup and teardown section first, then looking at each test section while still eventually reading the skip section from setup or teardown. We are also making SkipSection, SetupSection, TearDownSection, ClientYamlTestSection and ClientYamlTestSuite immutable. Previously it was possible to utilize constants like SetupSection.EMPTY, which were modifiable and affect every other future users by modifiying them. This has been corrected. Also, validation has been improved to cumulate errors so that all the errors from a suite will be listed at once. Relates to elastic#34735
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Oct 29, 2018
Validation of test section and suites consists of checking that the proper skip features sections are in place depending on the features used in tests. The validation logic was previously only performed on do sections included in each test section, and the skip needed to be present in the same test section. What happens often though is that the skip is added to the setup section, or the teardown section. This commit improves the validation of test suites by validating setup and teardown section first, then looking at each test section while still eventually reading the skip section from setup or teardown. We are also making SkipSection, SetupSection, TearDownSection, ClientYamlTestSection and ClientYamlTestSuite immutable. Previously it was possible to utilize constants like SetupSection.EMPTY, which were modifiable and affect every other future users by modifiying them. This has been corrected. Also, validation has been improved to cumulate errors so that all the errors from a suite will be listed at once. Relates to elastic#34735
javanna
added a commit
that referenced
this pull request
Oct 30, 2018
Validation of test sections and suites consists of checking that the proper skip features sections are in place depending on the features used in tests. The validation logic was previously only performed on do sections included in each test section, and the skip needed to be present in the same test section. What happens often though is that the skip is added to the setup section, or the teardown section. This commit improves the validation of test suites by validating setup and teardown section first, then looking at each test section while still eventually reading the skip section from setup or teardown. We are also making SkipSection, SetupSection, TearDownSection, ClientYamlTestSection and ClientYamlTestSuite immutable. Previously it was possible to utilize constants like SetupSection.EMPTY, which were modifiable and affect every other future users by modifiying them. This has been corrected. Also, validation has been improved to cumulate errors so that all the errors from a suite will be listed at once. Relates to #34735
javanna
added a commit
that referenced
this pull request
Nov 2, 2018
Validation of test sections and suites consists of checking that the proper skip features sections are in place depending on the features used in tests. The validation logic was previously only performed on do sections included in each test section, and the skip needed to be present in the same test section. What happens often though is that the skip is added to the setup section, or the teardown section. This commit improves the validation of test suites by validating setup and teardown section first, then looking at each test section while still eventually reading the skip section from setup or teardown. We are also making SkipSection, SetupSection, TearDownSection, ClientYamlTestSection and ClientYamlTestSuite immutable. Previously it was possible to utilize constants like SetupSection.EMPTY, which were modifiable and affect every other future users by modifiying them. This has been corrected. Also, validation has been improved to cumulate errors so that all the errors from a suite will be listed at once. Relates to #34735
javanna
added a commit
that referenced
this pull request
Nov 2, 2018
The java yaml test runner supports sending request headers, yet not all clients support headers. This commit makes sure that we enforce adding a skip section with feature "headers" whenever headers are used in a do section as part of a test. That decreases the chance for new tests to break client builds due to the missing skip section. Closes #34650
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The java yaml test runner supports sending request headers, yet not all clients support headers. This commit makes sure that we enforce adding a skip section with feature "headers" whenever headers are used in a do section as part of a test. That decreases the chance for new tests to break client builds due to the missing skip section.
Closes #34650