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

Fix flaky test by verifying that a search has been saved by checking for a success toast #21302

Closed
wants to merge 32 commits into from
Closed

Fix flaky test by verifying that a search has been saved by checking for a success toast #21302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
15466c2
Add testSubjects.existOrFail() helper. Verify a search has been saved…
cjcenizal Jul 26, 2018
af8089b
Apply pattern to other places where we're expecting success toasts.
cjcenizal Jul 26, 2018
3302926
Simplify existsOrFail.
cjcenizal Jul 26, 2018
eedebd0
Use terser testSubjects.click() method.
cjcenizal Jul 26, 2018
614cebd
Remove closeToast method. Adding a visualization confirms and closes …
cjcenizal Jul 26, 2018
5d19276
Remove CSS file.
cjcenizal Jul 26, 2018
6eedadf
Fix broken assertions about saveDashboard returning true. Remove unne…
cjcenizal Jul 27, 2018
6caf740
Add brief sleep to testSubjects.click to allow hidden elements to bec…
cjcenizal Jul 27, 2018
f3334ef
Fix broken assertions about clickCopyToClipboard returning true.
cjcenizal Jul 27, 2018
e752ec0
Add toasts service.
cjcenizal Jul 27, 2018
838e91e
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Jul 27, 2018
d6298a1
Fix dumb mistakes.
cjcenizal Jul 31, 2018
645a3c7
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Jul 31, 2018
4c26f4c
Fix RBAC tests by checking for presence of failure toast. Reduce exec…
cjcenizal Jul 31, 2018
8912ffe
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Jul 31, 2018
14e3e75
Wait for toast to be done animating in before dismissing it.
cjcenizal Jul 31, 2018
1260440
Skip flaky visualizations tests.
cjcenizal Jul 31, 2018
4b87d45
Fix dashboard tests which don't use saveDashboard method.
cjcenizal Aug 1, 2018
b05fd92
Fix flaky test by ignoring success toast due to heavy browser load.
cjcenizal Aug 1, 2018
9f74eef
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Aug 1, 2018
8e43781
Fix flaky full screen mode test by clicking on the button, not its text.
cjcenizal Aug 1, 2018
2b64fb8
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Aug 2, 2018
3847816
Rename ensureSaveSuccess to verifySaveSuccess.
cjcenizal Aug 2, 2018
10d7bfa
Re-enable linked saved searches test.
cjcenizal Aug 2, 2018
c953d42
Set absolute time in full screen mode test to allow it to be isolated…
cjcenizal Aug 3, 2018
7a5a82f
Add saveDashboardAndVerify method.
cjcenizal Aug 3, 2018
39bf935
Rename clickCopyToClipboard to clickCopyToClipboardAndVerify.
cjcenizal Aug 3, 2018
eb09cbe
Add saveVisualizationAndVerify method.
cjcenizal Aug 3, 2018
37e7864
Update reporting tests to use toasts service.
cjcenizal Aug 3, 2018
9998372
Temporarily remove all animations to see if this helps flakiness.
cjcenizal Aug 6, 2018
cb8212e
Use animation-duration instead of animation.
cjcenizal Aug 6, 2018
37ba74b
Merge remote-tracking branch 'upstream/master' into test-verify-saved…
cjcenizal Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions test/functional/page_objects/dashboard_page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,9 @@ export function DashboardPageProvider({ getService, getPageObjects }) {

await PageObjects.header.waitUntilLoadingHasFinished();

// Confirm that the Dashboard has been saved.
return await testSubjects.exists('saveDashboardSuccess');
// Confirm that the Dashboard has been saved and close the toast.
await testSubjects.existOrFail('saveDashboardSuccess');
await find.clickByCssSelector('[data-test-subj="saveDashboardSuccess"] [data-test-subj="toastCloseButton"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does using await testSubjects.click('saveDashboardSuccess toastCloseButton'); work? I see that style used somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, according to the tests for the test subj package, that should work.

}

async cancelSave() {
Expand Down
9 changes: 7 additions & 2 deletions test/functional/page_objects/discover_page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ export function DiscoverPageProvider({ getService, getPageObjects }) {
await getRemote().findDisplayedById('SaveSearch').pressKeys(searchName);
await testSubjects.click('discoverSaveSearchButton');
await PageObjects.header.waitUntilLoadingHasFinished();

// Make sure toast exists and then close it.
await testSubjects.existOrFail('saveSearchSuccess');
await find.clickByCssSelector('[data-test-subj="saveSearchSuccess"] [data-test-subj="toastCloseButton"]');
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the testSubjects.existOrFails are unnecessary because the find will fail if it doesn't find toast. Or do you prefer keeping them in for readability? Only concern is that they do disappear after awhile. I don't think it will cause issues but it would stink if it passed the first call, then failed on the second because in the interim, the toast disappeared. Unless we could do what we did with the old style toasts and modify them so they never disappear. Or maybe we have that? I'm not sure if the new toasts use the same advanced settings for how long to appear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I were reading the code and I saw this on its own:

await testSubject.click('saveSearchSuccess toastCloseButton');

It would not be clear to me why we're closing a toast that we haven't even looked at yet. The code implies the toast exists, but it doesn't verify this. I might even remove this code thinking that cleanup doesn't matter in this case, completely overlooking the role it plays as an assertion.

For me, the intent behind the code is clearer with two explicit actions: 1) the verification of success and 2) the removal of the toast to clean up the UI.

Only concern is that they do disappear after awhile

They'll disappear after a few seconds, but as soon as existsOrFail finds the toast, doesn't it resolve the promise? So there won't be any delay between these two actions, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

They'll disappear after a few seconds, but as soon as existsOrFail finds the toast, doesn't it resolve the promise?

yes

So there won't be any delay between these two actions, right?

Well, there is still a delay, just a very minor one, like, whatever time it takes the code to execute the next line.

The code implies the toast exists, but it doesn't verify this.

We don't add manual assertions that any given button exists, for instance, before we click it. Actually, we kind of do, but it's part of the click function, so we don't have to add a manual check before every click call. What this is doing is adding that manual call that already exists inside the click function.

I might even remove this code thinking that cleanup doesn't matter in this case, completely overlooking the role it plays as an assertion.

sgtm. We seem to be doing well with flakiness so maybe this doesn't do anything extra.

I guess the way I see it is the only thing this code does is add a very slight potential for extra flakiness by having a check for "existsOrFail" and then the subsequent click. What if, for instance, the "existsOrFail" function ends up searching through a bunch of DOM elements before it gets to the toast, so that function call takes a few seconds to return true, then before the next line can execute, the toast disappears. Unlikely (especially because the existsOfFail check takes max 1 s), but maybe possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't add manual assertions that any given button exists, for instance, before we click it. Actually, we kind of do, but it's part of the click function, so we don't have to add a manual check before every click call.

I think I understand how you're approaching this. I think you're looking at this from the angle of "How do we remove points of failure", which I totally get. I'm looking at it from the point of view of "How do we make this code clear to the reader", which is an orthogonal concern to yours.

What if, for instance, the "existsOrFail" function ends up searching through a bunch of DOM elements before it gets to the toast, so that function call takes a few seconds to return true, then before the next line can execute, the toast disappears.

I think what you're saying makes sense. I think this is possible and it's a valid concern. Though I'm not sure what's the best way to satisfy both of our concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code implies the toast exists, but it doesn't verify this. I might even remove this code thinking that cleanup doesn't matter in this case, completely overlooking the role it plays as an assertion.

Sorry, I read this wrong earlier. I thought you were saying you were actually going to remove the code you added because it doesn't matter.

I'm confused by your intentions with this PR. Initially I thought your goal was to avoid flakiness.
Then after re-reading your comment above, I thought your goal was to extend the tests. Now I'm re-reading the primary git issue comment, and it's indeed to fix a flaky test?

If you want to extend the tests to make them more robust (fail more often than they otherwise might now), I can see why you would add both lines (primarily want to make sure the toast is there, secondarily I want to dismiss it).

Or are you trying to reduce flakiness by first making sure the toast exists before dismissing it? In which case, the first line is pointless.

I'm just confused by what actually fixes the flakiness of the other test. You wrote

Fixes #20810 by asserting that the search has saved by checking for the presence of a success toast.

But that doesn't make sense to me, why would adding a more thorough check cause it to pass? If it does pass, it seems like it's accidental (like the extra check is similar to adding a little sleep and hence, you don't hit whatever you were hitting before).

I suppose it doesn't too much matter. You can check this code in and maybe it'll accidentally help with stability, but it seems like there is an underlying issue that might show up later.

}

async getColumnHeaders() {
Expand Down Expand Up @@ -221,8 +225,9 @@ export function DiscoverPageProvider({ getService, getPageObjects }) {
async clickCopyToClipboard() {
await testSubjects.click('sharedSnapshotCopyButton');

// Confirm that the content was copied to the clipboard.
return await testSubjects.exists('shareCopyToClipboardSuccess');
// Confirm that the content was copied to the clipboard and close the toast.
await testSubjects.existOrFail('shareCopyToClipboardSuccess');
await find.clickByCssSelector('[data-test-subj="shareCopyToClipboardSuccess"] [data-test-subj="toastCloseButton"]');
}

async getShareCaption() {
Expand Down
5 changes: 4 additions & 1 deletion test/functional/page_objects/visualize_page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,10 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
await testSubjects.click('saveAsNewCheckbox');
}
await PageObjects.header.waitUntilLoadingHasFinished();
return await testSubjects.exists('saveVisualizationSuccess');

// Confirm save and close toast.
await testSubjects.existOrFail('saveVisualizationSuccess');
await find.clickByCssSelector('[data-test-subj="saveVisualizationSuccess"] [data-test-subj="toastCloseButton"]');
}

async clickLoadSavedVisButton() {
Expand Down
7 changes: 6 additions & 1 deletion test/functional/services/dashboard/add_panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export function DashboardAddPanelProvider({ getService, getPageObjects }) {
const log = getService('log');
const retry = getService('retry');
const testSubjects = getService('testSubjects');
const find = getService('find');
const flyout = getService('flyout');
const PageObjects = getPageObjects(['header', 'common']);

Expand Down Expand Up @@ -144,7 +145,11 @@ export function DashboardAddPanelProvider({ getService, getPageObjects }) {
await this.filterEmbeddableNames(searchName);

await testSubjects.click(`addPanel${searchName.split(' ').join('-')}`);
await testSubjects.exists('addSavedSearchToDashboardSuccess');

// Confirm it was added and close the toast.
await testSubjects.existOrFail('addSavedSearchToDashboardSuccess');
await find.clickByCssSelector('[data-test-subj="addSavedSearchToDashboardSuccess"] [data-test-subj="toastCloseButton"]');

await this.closeAddPanel();
}

Expand Down
3 changes: 0 additions & 3 deletions test/functional/services/dashboard/visualizations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
export function DashboardVisualizationProvider({ getService, getPageObjects }) {
const log = getService('log');
const queryBar = getService('queryBar');
const testSubjects = getService('testSubjects');
const dashboardAddPanel = getService('dashboardAddPanel');
const PageObjects = getPageObjects(['dashboard', 'visualize', 'header', 'discover']);

Expand Down Expand Up @@ -56,8 +55,6 @@ export function DashboardVisualizationProvider({ getService, getPageObjects }) {
}

await PageObjects.discover.saveSearch(name);
await PageObjects.header.waitUntilLoadingHasFinished();
await testSubjects.exists('saveSearchSuccess');
Copy link
Contributor

Choose a reason for hiding this comment

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

how come no replacement with the dismissing of toast here?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh nm you probably moved it into saveSearch, haven't gotten there yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, PageObjects.discover.saveSearch() now checks for and closes the toast.

}

async createAndAddSavedSearch({ name, query, fields }) {
Expand Down
8 changes: 8 additions & 0 deletions test/functional/services/test_subjects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import expect from 'expect.js';
import testSubjSelector from '@kbn/test-subj-selector';
import {
filter as filterAsync,
Expand All @@ -37,6 +38,13 @@ export function TestSubjectsProvider({ getService }) {
return await find.existsByDisplayedByCssSelector(testSubjSelector(selector), timeout);
}

async existOrFail(selector, timeout = 1000) {
log.debug(`TestSubjects.existOrFail(${selector})`);
const doesExist = await this.exists(selector, timeout);
// Verify element exists, or else fail the test consuming this.
expect(doesExist).to.be(true);
}

async append(selector, text) {
return await retry.try(async () => {
const input = await this.find(selector);
Expand Down