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

[RAC] fix failing alerts table pagination functional tests #119985

Merged

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Nov 30, 2021

Summary

Fixes: #113486

Alerts table pagination functional tests were failing.

  • It was using WorkFlowFilter to test pagination. Since that filter has been removed, i replaced it with alertStatusFilter.
  • Modified the mock data to produce more meaningful number of active and recovered alerts.
  • Added a new method to check if the alert table data is being loaded and has been loaded, so test cases never can be flaky.

@ersin-erdal ersin-erdal added release_note:fix v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed Theme: rac label obsolete v8.1.0 Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Nov 30, 2021
@ersin-erdal ersin-erdal self-assigned this Nov 30, 2021
@ersin-erdal ersin-erdal force-pushed the 113486-fix-failing-pagination-tests branch from 0fcfc34 to 124eaeb Compare November 30, 2021 23:15
@ersin-erdal ersin-erdal marked this pull request as ready for review December 1, 2021 01:00
@ersin-erdal ersin-erdal requested a review from a team December 1, 2021 07:17
@@ -25,6 +25,12 @@ const ACTION_COLUMN_INDEX = 1;

type WorkflowStatus = 'open' | 'acknowledged' | 'closed';

export enum AlertStatus {
all = 'all',
active = 'active',
Copy link
Contributor

Choose a reason for hiding this comment

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

@ersin-erdal I suggest you import active and recovered values from https://github.com/elastic/kibana/blob/main/packages/kbn-rule-data-utils/src/alerts_as_data_status.ts

import {
  ALERT_STATUS_ACTIVE,
  ALERT_STATUS_RECOVERED,
} from '@kbn/rule-data-utils/alerts_as_data_status';

Copy link
Contributor Author

@ersin-erdal ersin-erdal Dec 1, 2021

Choose a reason for hiding this comment

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

i tried that but my IDE complained about it. it says that the folder is not in the root!
I think that's why the WorkflowStatus was also created there again.

Screenshot 2021-12-01 at 11 17 25

Copy link
Contributor Author

@ersin-erdal ersin-erdal Dec 1, 2021

Choose a reason for hiding this comment

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

Aaah, i imported it directly! that's why it complained...
Fixing it. Thank you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mgiota
Copy link
Contributor

mgiota commented Dec 1, 2021

@ersin-erdal LGTM! I left one comment regarding importing active and recovered from a kbn package.

@@ -192,7 +198,6 @@ export function ObservabilityAlertsCommonProvider({
const viewRuleDetailsLinkClick = async () => {
return await (await testSubjects.find(VIEW_RULE_DETAILS_FLYOUT_SELECTOR)).click();
};

// Workflow status
const setWorkflowStatusForRow = async (rowIndex: number, workflowStatus: WorkflowStatus) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ersin-erdal I see you removed workflow status related code. Do we still need this? Can we remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might require bigger refactoring, since this function might be used in other files as well. I live it up to you to decide if you want to do this refactoring as part of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i tried to get rid of it but a lot of functional tests failed! then i put it back.
It requires its own issue...

@@ -25,6 +25,12 @@ const ACTION_COLUMN_INDEX = 1;

type WorkflowStatus = 'open' | 'acknowledged' | 'closed';
Copy link
Contributor

Choose a reason for hiding this comment

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

@ersin-erdal Workflow status leftover. Can we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No unfortunately.
I tried to remove them as i said but setWorkflowStatusForRow and setWorkflowStatusFilter methods which requires that type are used in some other places...

Copy link
Contributor

Choose a reason for hiding this comment

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

@ersin-erdal I had the same issue. WorkflowStatus code is nested in many places.

@@ -266,6 +295,9 @@ export function ObservabilityAlertsCommonProvider({
setWorkflowStatusForRow,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above regarding remove workflowStatus leftovers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used by other tests... :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I can approve this PR and we could take care of left overs in a follow up PR. Looks like @fkanout already created an issue #119946

@mgiota mgiota self-requested a review December 1, 2021 10:53
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit 55430eb into elastic:main Dec 1, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
8.0 Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 119985

ersin-erdal added a commit to ersin-erdal/kibana that referenced this pull request Dec 1, 2021
…19985)

* [RAC] fix failing alerts table pagination functional tests
# Conflicts:
#	x-pack/test/observability_functional/apps/observability/alerts/index.ts
@ersin-erdal ersin-erdal deleted the 113486-fix-failing-pagination-tests branch December 1, 2021 13:36
ersin-erdal added a commit to ersin-erdal/kibana that referenced this pull request Dec 1, 2021
…19985)

* [RAC] fix failing alerts table pagination functional tests
# Conflicts:
#	x-pack/test/observability_functional/apps/observability/alerts/index.ts
ersin-erdal added a commit to ersin-erdal/kibana that referenced this pull request Dec 2, 2021
…19985)

* [RAC] fix failing alerts table pagination functional tests
ersin-erdal added a commit that referenced this pull request Dec 2, 2021
…120220)

* [RAC] fix failing alerts table pagination functional tests
@KOTungseth KOTungseth added the Team:APM All issues that need APM UI Team support label Dec 8, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
…19985)

* [RAC] fix failing alerts table pagination functional tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Team:APM All issues that need APM UI Team support Theme: rac label obsolete v8.0.0 v8.1.0
Projects
None yet
7 participants