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

[pageObjects/dashboard] check that save is complete before resolving #21892

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 10, 2018

While debugging failures I saw in #21772 I found myself encountering failure messages like Error: expected undefined to sort of equal true, and other more cryptic errors caused by methods like PageObjects.dashboard.saveDashboard() not ensuring that the dashboard was actually saved before resolving. As part of the debugging effort I noticed that the saveDashboard() method does have some awareness of the success condition, but rather than asserting success within the method it returns a success boolean for the caller to check, which was only being done in a handful of tests in test/functional/apps/dashboard/_dashboard_time.js but was ignored the vast majority of the time.

I think that most of the time we are calling PageObjects.dashboard.saveDashboard() we correctly assume that if the dashboard couldn't be saved for some reason the promise will be rejected and the test would fail. If the method was called maybeSaveDashboard() or tryToSaveDashboard() there might be a signal to consumers that they should check for success conditions, but that would also lead to the same checks all over the place. Instead, this PR reverses the responsibility of checking for success so that code calling PageObjects.dashboard.saveDashboard() can continue to assume that if something went wrong their test will fail. It also improves the error message by not using expect(boolean).to.equal(boolean), instead implementing a basic if() statement and throwing an error with a meaningful message when something goes wrong.

const isDashboardSaved = await testSubjects.exists('saveDashboardSuccess');
expect(isDashboardSaved).to.eql(true);

is now

if (!await testSubjects.exists('saveDashboardSuccess')) {
  throw new Error('Expected to find "saveDashboardSuccess" toast after saving dashboard');
}

I think this type of change could be made to a lot of methods, and would make failures a lot easier to debug and possibly a lot less flaky if we were checking for success conditions in nearly every method we put in our PageObjects. I think it's safe to say that most of the methods we have in PageObjects do not check for actual success criteria, and sometimes that's okay: a method called clickButton() can safely resolve once the click method has been called, but a method like addSampleDataSet() should be verifying that the sample data set it set out to add was actually added.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor

retest

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

did not pull down (relying on ci) but changes make sense to me. LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese 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 review

@spalger spalger merged commit 359ac43 into elastic:master Aug 13, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 13, 2018
…lastic#21892)

While debugging failures I saw in elastic#21772 I found myself encountering failure messages like `Error: expected undefined to sort of equal true`, and other more cryptic errors caused by methods like `PageObjects.dashboard.saveDashboard()` not ensuring that the dashboard was actually saved before resolving. As part of the debugging effort I noticed that the `saveDashboard()` method does have some awareness of the success condition, but rather than asserting success within the method it returns a success boolean for the caller to check, which was only being done in a handful of tests in `test/functional/apps/dashboard/_dashboard_time.js` but was ignored the vast majority of the time.

I think that most of the time we are calling `PageObjects.dashboard.saveDashboard()` we correctly assume that if the dashboard couldn't be saved for some reason the promise will be rejected and the test would fail. If the method was called `maybeSaveDashboard()` or `tryToSaveDashboard()` there might be a signal to consumers that they should check for success conditions, but that would also lead to the same checks all over the place. Instead, this PR reverses the responsibility of checking for success so that code calling `PageObjects.dashboard.saveDashboard()` can continue to assume that if something went wrong their test will fail. It also improves the error message by not using `expect(boolean).to.equal(boolean)`, instead implementing a basic `if()` statement and throwing an error with a meaningful message when something goes wrong.

```js
const isDashboardSaved = await testSubjects.exists('saveDashboardSuccess');
expect(isDashboardSaved).to.eql(true);
```

is now

```js
if (!await testSubjects.exists('saveDashboardSuccess')) {
  throw new Error('Expected to find "saveDashboardSuccess" toast after saving dashboard');
}
```

---

I think this type of change could be made to a lot of methods, and would make failures a lot easier to debug and possibly a lot less flaky if we were checking for success conditions in nearly every method we put in our PageObjects. I think it's safe to say that most of the methods we have in PageObjects do not check for actual success criteria, and sometimes that's okay: a method called `clickButton()` can safely resolve once the click method has been called, but a method like `addSampleDataSet()` should be verifying that the sample data set it set out to add was actually added.
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 13, 2018
…lastic#21892)

While debugging failures I saw in elastic#21772 I found myself encountering failure messages like `Error: expected undefined to sort of equal true`, and other more cryptic errors caused by methods like `PageObjects.dashboard.saveDashboard()` not ensuring that the dashboard was actually saved before resolving. As part of the debugging effort I noticed that the `saveDashboard()` method does have some awareness of the success condition, but rather than asserting success within the method it returns a success boolean for the caller to check, which was only being done in a handful of tests in `test/functional/apps/dashboard/_dashboard_time.js` but was ignored the vast majority of the time.

I think that most of the time we are calling `PageObjects.dashboard.saveDashboard()` we correctly assume that if the dashboard couldn't be saved for some reason the promise will be rejected and the test would fail. If the method was called `maybeSaveDashboard()` or `tryToSaveDashboard()` there might be a signal to consumers that they should check for success conditions, but that would also lead to the same checks all over the place. Instead, this PR reverses the responsibility of checking for success so that code calling `PageObjects.dashboard.saveDashboard()` can continue to assume that if something went wrong their test will fail. It also improves the error message by not using `expect(boolean).to.equal(boolean)`, instead implementing a basic `if()` statement and throwing an error with a meaningful message when something goes wrong.

```js
const isDashboardSaved = await testSubjects.exists('saveDashboardSuccess');
expect(isDashboardSaved).to.eql(true);
```

is now

```js
if (!await testSubjects.exists('saveDashboardSuccess')) {
  throw new Error('Expected to find "saveDashboardSuccess" toast after saving dashboard');
}
```

---

I think this type of change could be made to a lot of methods, and would make failures a lot easier to debug and possibly a lot less flaky if we were checking for success conditions in nearly every method we put in our PageObjects. I think it's safe to say that most of the methods we have in PageObjects do not check for actual success criteria, and sometimes that's okay: a method called `clickButton()` can safely resolve once the click method has been called, but a method like `addSampleDataSet()` should be verifying that the sample data set it set out to add was actually added.
spalger pushed a commit that referenced this pull request Aug 14, 2018
…lving (#21892) (#21945)

Backports the following commits to 6.x:
 - [pageObjects/dashboard] check that save is complete before resolving  (#21892)
spalger pushed a commit that referenced this pull request Aug 14, 2018
…lving (#21892) (#21946)

Backports the following commits to 6.4:
 - [pageObjects/dashboard] check that save is complete before resolving  (#21892)
@spalger
Copy link
Contributor Author

spalger commented Aug 14, 2018

6.x/6.5: 05cba5a
6.4: 124a351

@spalger spalger deleted the implement/pageObjects/dashboard/checkForSaveComplete branch August 14, 2018 00:22
@cjcenizal
Copy link
Contributor

Similar to #21302

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.

5 participants