Skip to content

Comments

[retry] implement waitFor method#21747

Merged
spalger merged 3 commits intoelastic:masterfrom
spalger:implement/retry-waitFor
Aug 14, 2018
Merged

[retry] implement waitFor method#21747
spalger merged 3 commits intoelastic:masterfrom
spalger:implement/retry-waitFor

Conversation

@spalger
Copy link
Contributor

@spalger spalger commented Aug 7, 2018

We currently use the retry service to call a function over and over in a loop, waiting for it to run without throwing an error, and ultimately failing if it does not succeed before a timeout is exceeded. This is the easiest way to get certain interactions to work, either because we don't know when we should be able to execute the interaction successfully, or because the timing is just too tricky to plan out correctly. Another place where we are using the retry service, which I don't think is appropriate, is when we need to wait for a certain condition to be met. This is where retry.waitFor() comes in:

await retry.waitFor('dashboard search to be enabled', async () => {
  const searchInput = await testSubjects.find('savedObjectFinderSearchInput');
  return await searchInput.isEnabled();
})

The retry.waitFor() method behaves much like the retry.try() method behind the scenes, calling a function over and over, but instead of waiting for it to run without throwing an error it waits for it to return a "truthy" value. It also requires a description string that is used to make rather nice log output and a more descriptive error message than something like https://github.com/elastic/kibana/blob/master/test/functional/apps/dashboard/_data_shared_attributes.js#L61-L67

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@LeeDr
Copy link

LeeDr commented Aug 7, 2018

Would retry.waitFor work with expect statements? We don't return await those. But they're a very common thing we would want to wait for.

@spalger
Copy link
Contributor Author

spalger commented Aug 8, 2018

waitFor replaces these types of expect statements:

retry.try(async () => {
  // ...
  expect(someBoolean).to.be(true)
})

with

retry.waitFor('some boolean', async () => {
  // ...
  return someBoolean
})

Retry's that are checking other things might be converted like so:

retry.waitFor('all visualizations to render', async () => {
  const renderedCount = await PageObjects.dashboard.countRenderedVisualizations();
  return renderedCount === expectedAmount
})

using waitFor here instead of retry.try() will mostly be beneficial for the better logging and readability of the code.

@spalger
Copy link
Contributor Author

spalger commented Aug 8, 2018

You can see an example implementation that's actually improving the stability of the tests in #21629 here: https://github.com/elastic/kibana/blob/7cb23e9d1dd4aeba3c6869b3b6ddbd93e58c74c6/test/functional/page_objects/dashboard_page.js#L179-L181

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - code reviewed only. Passing in Jenkins.

@spalger spalger merged commit 3b8e957 into elastic:master Aug 14, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 14, 2018
We currently use the `retry` service to call a function over and over in a loop, waiting for it to run without throwing an error, and ultimately failing if it does not succeed before a timeout is exceeded. This is the easiest way to get certain interactions to work, either because we don't know when we should be able to execute the interaction successfully, or because the timing is just too tricky to plan out correctly. Another place where we are using the `retry` service, which I don't think is appropriate, is when we need to wait for a certain condition to be met. This is where `retry.waitFor()` comes in:

```js
await retry.waitFor('dashboard search to be enabled', async () => {
  const searchInput = await testSubjects.find('savedObjectFinderSearchInput');
  return await searchInput.isEnabled();
})
```

The `retry.waitFor()` method behaves much like the `retry.try()` method behind the scenes, calling a function over and over, but instead of waiting for it to run without throwing an error it waits for it to return a "truthy" value. It also requires a description string that is used to make rather nice log output and a more descriptive error message than something like https://github.com/elastic/kibana/blob/master/test/functional/apps/dashboard/_data_shared_attributes.js#L61-L67
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 14, 2018
We currently use the `retry` service to call a function over and over in a loop, waiting for it to run without throwing an error, and ultimately failing if it does not succeed before a timeout is exceeded. This is the easiest way to get certain interactions to work, either because we don't know when we should be able to execute the interaction successfully, or because the timing is just too tricky to plan out correctly. Another place where we are using the `retry` service, which I don't think is appropriate, is when we need to wait for a certain condition to be met. This is where `retry.waitFor()` comes in:

```js
await retry.waitFor('dashboard search to be enabled', async () => {
  const searchInput = await testSubjects.find('savedObjectFinderSearchInput');
  return await searchInput.isEnabled();
})
```

The `retry.waitFor()` method behaves much like the `retry.try()` method behind the scenes, calling a function over and over, but instead of waiting for it to run without throwing an error it waits for it to return a "truthy" value. It also requires a description string that is used to make rather nice log output and a more descriptive error message than something like https://github.com/elastic/kibana/blob/master/test/functional/apps/dashboard/_data_shared_attributes.js#L61-L67
spalger pushed a commit that referenced this pull request Aug 14, 2018
Backports the following commits to 6.4:
 - [retry] implement waitFor method  (#21747)
spalger pushed a commit that referenced this pull request Aug 14, 2018
Backports the following commits to 6.x:
 - [retry] implement waitFor method  (#21747)
@spalger
Copy link
Contributor Author

spalger commented Aug 14, 2018

6.5/6.x: 0072315
6.4: a6296dc

@spalger spalger deleted the implement/retry-waitFor branch August 14, 2018 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants