Fix flaky test by verifying that a search has been saved by checking for a success toast #21302
Conversation
… by checking for and closing the success toast.
| 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"]'); |
There was a problem hiding this comment.
Does using await testSubjects.click('saveDashboardSuccess toastCloseButton'); work? I see that style used somewhere else.
There was a problem hiding this comment.
Good call, according to the tests for the test subj package, that should work.
|
|
||
| // Make sure toast exists and then close it. | ||
| await testSubjects.existOrFail('saveSearchSuccess'); | ||
| await find.clickByCssSelector('[data-test-subj="saveSearchSuccess"] [data-test-subj="toastCloseButton"]'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
💔 Build Failed |
💔 Build Failed |
…its own success toast.
| @@ -329,12 +329,6 @@ export function CommonPageProvider({ getService, getPageObjects }) { | |||
| }); | |||
| } | |||
|
|
|||
| async closeToast() { | |||
There was a problem hiding this comment.
I think we should strive for very specific, intentional actions. A "toast" on its own is meaningless... what matters it what the toast represents, e.g. success, failure, etc. As we make changes to our UX we could replace this toast with a callout or a checkmark icon somewhere in the UI. A test which clicks a button and then looks for a toast representative of a successful outcome will be easier to understand and update than a test which just closes an open toast.
💔 Build Failed |
|
|
Retest |
| @@ -56,8 +55,6 @@ export function DashboardVisualizationProvider({ getService, getPageObjects }) { | |||
| } | |||
|
|
|||
| await PageObjects.discover.saveSearch(name); | |||
| await PageObjects.header.waitUntilLoadingHasFinished(); | |||
| await testSubjects.exists('saveSearchSuccess'); | |||
There was a problem hiding this comment.
how come no replacement with the dismissing of toast here?
There was a problem hiding this comment.
oh nm you probably moved it into saveSearch, haven't gotten there yet.
There was a problem hiding this comment.
Right, PageObjects.discover.saveSearch() now checks for and closes the toast.
|
@stacey-gammon Sorry, I wasn't clear about how I think this PR addresses the original issue. I think this removes the flakiness reported in the original issue by waiting for the success toast to verify the search has been saved before proceeding to the assertion on the query name. My theory is that the I think this PR then snowballed a bit when I checked for this pattern elsewhere and found the Does this make sense? |
💔 Build Failed |
|
Looks like a legit failure related to my changes. The goal of this week is to create flaky tests, right?!
|
…cessary waitUntilLoadingHasFinished calls where toasts are now used to verify success.
💔 Build Failed |
|
Getting there...
|
…ome visible (and clickable).
|
These last two fixes have been interesting. The first one required me to remove the explicit assertions in the tests in favor of the implicit ones in the The second one was a doozy. I watched the test execute locally and what happens is the toast shows up, the close button becomes visible (indicating the mouse is hovering over the toast), but then nothing happens -- the toast is never closed. When run in verbose mode, the output shows the click request is sent through and gets back an OK response. Looking through the toast code, it's possible that the click is being ignored by the component. I need to dig into this before merging this PR -- if it's something in the toast then maybe we can solve this in EUI instead of in our functional tests. For the time being, waiting a few ms before clicking the toast solved the problem so at least we can see if we can get a successful CI run. |
There was a problem hiding this comment.
Left some comments. Not sure that we should merge this if we suspect it has caused other test failures unless we fix those failures or can demonstrate they are unrelated. Also FYI my github account is bmcconaghy. billm is probably wondering why he got tagged on this ;-)
| const isDashboardSaved = await PageObjects.dashboard.saveDashboard(dashboardName, { storeTimeWithDashboard: false }); | ||
| expect(isDashboardSaved).to.eql(true); | ||
| // saveDashboard asserts that it saves successfully. | ||
| await PageObjects.dashboard.saveDashboard(dashboardName, { storeTimeWithDashboard: false }); |
There was a problem hiding this comment.
Maybe rename the saveDashboard method to make it clear that it ensures the save, since the same comment is used in more than one place to indicate that?
| const isCopiedToClipboard = await PageObjects.discover.clickCopyToClipboard(); | ||
| expect(isCopiedToClipboard).to.eql(true); | ||
| // This method is self-confirming, so we don't need an assertion. | ||
| await PageObjects.discover.clickCopyToClipboard(); |
There was a problem hiding this comment.
same as above, maybe indicate the self-confirming nature of this method in its name.
…. Ensure exit full screen button exists before clicking it.
💚 Build Succeeded |
|
Thanks for the review |
cjcenizal
force-pushed
the
test-verify-saved-search
branch
from
dc3a930
to
eb09cbe
Compare
|
I'm with @bmcconaghy - I don't think we should merge this unless we are sure it didn't cause the other flakiness. Now that @spalger added the build to track failures, we can better see if a test is flaky in master or here. Alternatively we could create a PR to run the other suspected flaky tests 20x and see if it hits any failures. |
💚 Build Succeeded |
|
Retest |
💚 Build Succeeded |
💔 Build Failed |
|
I'm giving up on trying to account for animation within the functional tests themselves. I think that @spalger's direction in #21629 holds more promise. I just pushed a commit which removes all animations (modals, flyouts, toasts) to see if this reduces flakiness. If it does, then I'll block this PR on his. |
💔 Build Failed |
💔 Build Failed |
|
This one again:
|
|
#21704 will fix this ^ |
💔 Build Failed |
Fixes #20810 and #19750 by asserting that the search has saved by checking for the presence of a success toast. The flaky test then passed locally for me.