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

Run more search Rest tests in a CCS setup #86521

Merged
merged 14 commits into from
May 24, 2022

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented May 6, 2022

Currently we only test a small subset of cross cluster search in rest tests in the 'multi-cluster-search' qa module. In order to increase test coverage for basic CCS setups, this change adds a new qa modula that uses a subset of existing search rest tests tests and runs them in a CCS scenario.

Relates to #84481

@cbuescher cbuescher added the WIP label May 6, 2022
@cbuescher cbuescher mentioned this pull request May 6, 2022
5 tasks
@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label May 6, 2022
@elasticmachine
Copy link
Collaborator

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

@cbuescher cbuescher force-pushed the ccs-common-rest branch 4 times, most recently from 52b1fe9 to 5c1eb3a Compare May 10, 2022 14:52
@cbuescher cbuescher changed the title WIP: Run search rest tests in CCS setup Run more search Rest tests in a CCS setup May 10, 2022
@cbuescher cbuescher added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories labels May 10, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label May 10, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Currently we only test a small subset of cross cluster search in rest tests in
the 'multi-cluster-search' qa module. In order to increase test coverage for
basic CCS setups, this change adds a new qa modula that uses a subset of
existing search rest tests tests and runs them in a CCS scenario.

Relates to elastic#84481
@cbuescher
Copy link
Member Author

Some of the ccs compatible endpoints (like e.g. search_template) have their yaml rest tests under "modules" or x-pack (terms_enum), I don't yet if/how we can run them from here, so this should probably be a follow up task.

@cbuescher cbuescher requested a review from javanna May 16, 2022 14:53
@cbuescher
Copy link
Member Author

I added the "indices.resolve_index" api test to the covered subset of rest tests, also fixed a small issue that led to some test assertions being ignored where they shouldn't have, which led to a few overlooked places in "field_caps" where the testrunner still needed to rewrite the expected matches. Locally 751 tests are passing now, with only the blacklisted ones skipped atm.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I left a couple of questions, this looks right to me though, thanks for working on this!

qa/ccs-common-rest/build.gradle Outdated Show resolved Hide resolved
qa/ccs-common-rest/build.gradle Outdated Show resolved Hide resolved
qa/ccs-common-rest/build.gradle Outdated Show resolved Hide resolved
qa/ccs-common-rest/build.gradle Outdated Show resolved Hide resolved
qa/ccs-common-rest/build.gradle Outdated Show resolved Hide resolved
@cbuescher
Copy link
Member Author

@javanna I pushed an update adressing your comments. I can also add the "_search_shard" endpoint to the APIs we route to the local search cluster instead of the remote holding the data if that API is supposed to support CCS and we want to include it in the testing here.

@javanna
Copy link
Member

javanna commented May 19, 2022

@cbuescher I do not think that the search shards API supports cross-cluster calls, does it?

@cbuescher
Copy link
Member Author

No, I also think it doesn't. So nothing to do there then.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM


systemProperty 'tests.rest.blacklist',
[
// TODO look into fixing these
Copy link
Member

Choose a reason for hiding this comment

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

looks like the reasons why these are in the blacklist are good, so maybe the TODO can be removed?

@cbuescher
Copy link
Member Author

@javanna thanks for the heads up. I think I can include "_terms_enum" and "async_search" yaml tests from xpack before merging, might need another few commits on this PR though.

@cbuescher
Copy link
Member Author

@javanna just fyi I added the "asych_search" tests from x-pack to be included in the suites we are running here. Adding "terms_enum" in a similar way proved to be difficult since the basic tests under "x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/terms_enum/10_basic.yml" require a security setup which we don't have here.
But then again there are rest tests covering "terms_enum" and CCS including security under "x-pack/qa/multi-cluster-search-security/src/test/resources/rest-api-spec/test/multi_cluster/120_terms_enum.yml" so I think we are covered here.

@cbuescher cbuescher merged commit 410d381 into elastic:master May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Clients Meta label for clients team Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants