[Actionable Observability] Integrate the shareable alert table in the Overview Page #140024
Conversation
maryam-saeidi
added
chore
Team: Actionable Observability - DEPRECATED
maryam-saeidi
force-pushed
the
135130-shared-alert-table-overview-page-integration
branch
from
baf3e1e
to
ab66810
Compare
|
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
| describe('Without alerts', function () { | ||
| it('navigate and open alerts section', async () => { | ||
| await observability.overview.common.navigateToOverviewPage(); | ||
| await PageObjects.header.waitUntilLoadingHasFinished(); | ||
| await observability.overview.common.openAlertsSectionAndWaitToAppear(); | ||
| }); | ||
|
|
||
| it('should show no data message', async () => { | ||
| await retry.try(async () => { | ||
| await observability.overview.common.getAlertsTableNoDataOrFail(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
If I understand correctly, the first test case navigates to the overview page and asserts the alerts section opens, while the second test case asserts on no data in the alerts table.
But the second test case does not seem to navigate to the overview page.
I might not understand totally how the functional tests are setup, but I think tests should not depend on a state set by another test.
I think the order of execution is not guaranteed, and therefore the 2nd test might run before the first one. And even if the order of execution is guaranteed, if someone adds a new test in between, it might make the next test fail.
I suggest to write the tests as they were alone with all their necessary setup, e.g. navigating to the overview page, waiting for the headers, etc...
In this case we could also just merge the two test cases into one? What do you think?
Also, this might explain why you had some flakiness earlier.
There was a problem hiding this comment.
Hey Kevin,
Thanks for your comment. Indeed tests are related to each other and we cannot change order or insert a step in between without ensuring everything will work fine in the new setup.
This is the expected behavior in functional tests, functional tests are like a narrative of the user's interaction with our system and the test is set up in a way that it will not continue without first making sure previous steps are successful. As a result of this setup, we cannot run one test case in isolation and we need to run the whole test suite.
The flakiness is related to await observability.overview.common.openAlertsSectionAndWaitToAppear(); and I will test another idea to make sure it will not cause any issue in future :)
| }); | ||
| }); | ||
|
|
||
| describe('With alerts', function () { |
There was a problem hiding this comment.
I have the same comment as above
There was a problem hiding this comment.
UX/UI generally looks good. I did find some possible bugs and enhancements.
I found that when moving between alert pages, it will show a bunch of empty items from the previous page - then drop them. If I refresh, it comes back.
CleanShot.2022-09-09.at.13.57.45.mp4
Also opened #140387 to look at fixing the double loading states for the alerts panel.
|
cc @XavierM 👆🏻 |
|
@formgeist Thanks for finding that bug, that bother me too when I saw it. So I created an issue and PR to fix it #140440 For the double loading state, I think we should discuss more because I do not think that it is super easy to fix. The problem is the alert table is a lazy component which it is using react.lazy/suspense to be loaded. We are doing that to avoid exploding out our bundle size. So the first loading state that you see, should only happen one time when the page get loaded with the alert table component and then you should never see it again. However I do agree, it will be nice to find a cleaner and more elegant solution. |
Great, thanks for taking a look so quickly!
OK, makes sense. Let's continue to discuss in #140387 |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
maryam-saeidi
deleted the
135130-shared-alert-table-overview-page-integration
branch
kibanamachine
added
v8.5.0
backport:skip
Resolves #135130
Summary
You should be able to see the new table on the overview page and the time filter should be applied to it correctly.
Known issue
Fullscreen has an issue that you see in the following screenshot. I will investigate it later and I might create a separate ticket for it. (I've decided to tackle it separately, ticket: #140389)
Checklist