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.

Sign up for GitHub

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

Jump to bottom

Unskip search api tests #123145

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Unskip search api tests #123145

merged 4 commits into from
Jan 19, 2022

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 17, 2022 •
edited
Loading

Dosant
Dosant commented Jan 17, 2022
View reviewed changes
x-pack/test/api_integration/apis/search/search.ts
const shardDelayAgg = (delay: string) => ({
aggs: {
delay: {
shard_delay: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using dev only shard_delay agg to stabilize checks the depend on how much time it took to get the result

x-pack/test/api_integration/apis/search/search.ts
describe.skip('post', () => {
it('should return 200 with final response if wait_for_completion_timeout is long enough', async () => {
before(async () => {
// ensure es not empty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that shard_delay doesn't kick with empty es

@Dosant Dosant added Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:AppServicesSv v8.1.0 labels Jan 17, 2022
@Dosant Dosant changed the title Unksip search api tests Unskip search api tests Jan 17, 2022
@Dosant Dosant marked this pull request as ready for review January 17, 2022 14:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@Dosant Dosant requested review from lukasolson and a team January 17, 2022 17:27
lukasolson
lukasolson reviewed Jan 18, 2022
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Our two tests here are "should delete a search" and "should retrieve results with id". I'm curious as to what you find as the advantages to introducing the shard delay into these requests rather than simply adding the keep_on_completion parameter in these requests.

Either way, I see the value in providing these utilities. Maybe we could add a couple of additional tests, so that we have all of the following test cases:

  • Can retrieve in-progress search by ID
  • Can retrieve completed search by ID
  • Can delete in-progress search
  • Can delete completed search

@Dosant
Copy link
Contributor Author

Dosant commented Jan 19, 2022

Our two tests here are "should delete a search" and "should retrieve results with id". I'm curious as to what you find as the advantages to introducing the shard delay into these requests rather than simply adding the keep_on_completion parameter in these requests.

I thought that adding a delay is closer to the real-world usage

@Dosant
Copy link
Contributor Author

Dosant commented Jan 19, 2022

@elasticmachine merge upstream

@Dosant Dosant requested a review from lukasolson January 19, 2022 14:02
@Dosant
Copy link
Contributor Author

Dosant commented Jan 19, 2022

@lukasolson, I've added more cases 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lukasolson
lukasolson approved these changes Jan 19, 2022
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM

@Dosant Dosant merged commit d84a5df into elastic:main Jan 19, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 19, 2022
lukasolson pushed a commit to lukasolson/kibana that referenced this pull request Jan 27, 2022
@Dosant @lukasolson
Unskip search api tests (elastic#123145)
0eb585f 
(cherry picked from commit d84a5df)
@lukasolson lukasolson mentioned this pull request Jan 27, 2022
Merged
lukasolson pushed a commit to lukasolson/kibana that referenced this pull request Jan 27, 2022
@Dosant @lukasolson
Unskip search api tests (elastic#123145)
b1511d7 
(cherry picked from commit d84a5df)

# Conflicts:
#	x-pack/test/api_integration/apis/search/search.ts
@lukasolson lukasolson mentioned this pull request Jan 27, 2022
Merged
lukasolson added a commit that referenced this pull request Jan 27, 2022
@lukasolson @Dosant
Unskip search api tests (#123145) (#123958)
fc23293 
(cherry picked from commit d84a5df)

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
lukasolson added a commit that referenced this pull request Jan 31, 2022
@lukasolson @Dosant
[7.17] Unskip search api tests (#123145) (#123959)
26b494a 
* Unskip search api tests (#123145)

(cherry picked from commit d84a5df)

# Conflicts:
#	x-pack/test/api_integration/apis/search/search.ts

* Fix backport

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasolson lukasolson lukasolson approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v8.1.0
Projects
None yet
Milestone
No milestone
5 participants
@Dosant @elasticmachine @kibana-ci @lukasolson @kibanamachine