Skip to content
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 _xpack prefix compat tests to their own qa module #79501

Merged
merged 10 commits into from
Oct 25, 2021

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Oct 19, 2021

This commit moves where we the _xpack/* REST compatible tests are executed.
The _xpack/* prefix was deprecated in 7.x and removed in 8.x via #35958.
However, when deprecated the paths were also removed from the API spec in 7.x.
This means that these paths are not available to test with the YAML test framework.

As part of the work to support REST API compatibility for 8.x these _xpack prefix paths
were re-introduced when using compatibility. These paths were tested via the primary
x-pack YAML rest tests using custom specs and transformations. These custom specs are
needed to allow the testing framing work to use the _xpack/* paths and the transformations
tell the tests to use the _xpack prefix. To avoid re-introducing these prefix paths in the 7.x spec,
custom specs for the tests are used.

This worked well for testing on the server, but after introducing the compatibility
tests via the rest-resources-zip (#78534) the transformed tests in that zip
requires those custom specs to execute properly. Since these are re-introduced just
for testing we do not want to add these to the zip file of specs.

To allow the xpack prefix to continue to be tested AND allow the tests to execute
as-is via the specs/tests in the rest-resource-zip BUT do not expose the need for
custom specs in the zip file ; these tests have been moved to their own qa module.

As an implementation detail, the tests in the new qa module are transformed twice. Once
as determined by the main x-pack transforms for things like compatible type support, and
then once again solely for the purpose for testing the xpack prefix.

@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Oct 19, 2021
@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

@jakelandis jakelandis force-pushed the move_xpack_prefix_testing branch from cf20a5e to 792ffdd Compare October 19, 2021 18:52
@jakelandis jakelandis changed the title Work in progress [REST API compatibility] Move _xpack prefix compat tests to their own qa module Oct 20, 2021
@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

@jakelandis jakelandis added :Delivery/Build Build or test infrastructure >non-issue labels Oct 20, 2021
@jakelandis jakelandis marked this pull request as ready for review October 20, 2021 14:19
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Oct 20, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

@mark-vieira
Copy link
Contributor

To allow the xpack prefix to continue to be tested AND allow the tests to execute as-is via the specs/tests in the rest-resource-zip BUT do not expose the need for custom specs in the zip file ; these tests have been moved to their own qa module.

Moving them means they will no longer be included in the rest-resource-zip though, unless I'm missing something. That artifact contains only "core" tests, that is the stuff in the :rest-api-spec project as well as x-pack/plugin/src.

@jakelandis
Copy link
Contributor Author

Moving them means they will no longer be included in the rest-resource-zip though, unless I'm missing something.

There are 2 sets of normal YAML tests (core and xpack from master), and 2 sets of compatible tests (core and xpack from 7.x) included in the zip (before and after this change). Before this change the xpack tests from 7.x were transformed to run the tests using the _xpack prefix. After this change they will no longer be transformed and the main xpack compatible tests will no longer test using the _xpack prefix.

There is no need for the clients to test against the _xpack prefix since it doesn't actually do anything except offer an old path that was removed from the spec a long time ago. The server however should test that old path to make sure we don't accidentally remove those paths. This new module does just that, it runs a subset of the xpack compatible tests but with an additional transform to ensure they hit the old paths. The specs are an implementation detail needed by the tests to allow them to hit the old paths. Ideally, (imo) a long time ago, they should not have removed from the spec since the prefix path was still valid, but they were removed and we don't want to re-introduce them. If they were never removed from the spec this would not be an issue.

There is 1 single test that might actually be removed from the zip - https://github.com/elastic/elasticsearch/pull/79501/files#diff-ed22f457ba4b2f21ce220d649b07a86b452bbacc37e9ec13facbddc85f30b9d5 which is only there to test the old path since the is not any other tests (that are enabled) that can be transformed to test this. The rest of the things moved are the the specs that we do not want in the zip file.

@jakelandis
Copy link
Contributor Author

@mark-vieira - should be ready for another look. The duplicate task has been cleaned up fde2149 (#79501)

@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM

@jakelandis jakelandis merged commit bfd8e15 into elastic:master Oct 25, 2021
@jakelandis jakelandis deleted the move_xpack_prefix_testing branch October 25, 2021 22:04
lockewritesdocs pushed a commit to lockewritesdocs/elasticsearch that referenced this pull request Oct 28, 2021
… qa module (elastic#79501)

This commit moves where we the _xpack/* REST compatible tests are executed.
The _xpack/* prefix was deprecated in 7x and removed in 8.x via elastic#35958.
However, when deprecated the paths were also removed from the API spec in 7x.
This means that these paths are not available to test with the YAML test framework.

As part of the work to support REST API compatibility for 8.x these _xpack prefix paths
were re-introduced when using compatibility. These paths were tested via the primary
x-pack YAML rest tests using custom specs and transformations. These custom specs are
needed to allow the testing framing work to use the _xpack/* paths and the transformations
tell the tests to use the _xpack prefix. To avoid re-introducing these prefix paths in the 7.x spec,
custom specs for the tests are used.

This worked well for testing on the server, but after introducing the compatibility
tests via the rest-resources-zip (elastic#78534) the transformed tests in that zip
requires those custom specs to execute properly. Since these are re-introduced just
for testing we do not want to add these to the zip file of specs.

To allow the xpack prefix to continue to be tested AND allow the tests to execute
as-is via the specs/tests in the rest-resource-zip BUT do not expose the need for
custom specs in the zip file ; these tests have been moved to their own qa module.

As an implementation detail, the tests in the new qa module are transformed twice. Once
as determined by the main x-pack transforms for things like compatible type support, and
then once again solely for the purpose for testing the xpack prefix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Clients Meta label for clients team Team:Delivery Meta label for Delivery team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants