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

[Uptime] Unskip alerting functional tests #72963

Merged
merged 10 commits into from
Jul 31, 2020

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Jul 22, 2020

Summary

Fixes #65948.

WIP, fixing skipped tests in the Uptime alerting test suite.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience test_xpack_functional v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v7.9.0 labels Jul 22, 2020
@justinkambic justinkambic self-assigned this Jul 22, 2020
@justinkambic justinkambic force-pushed the uptime_unskip-alerting-tests branch from c515c09 to ba78cdf Compare July 23, 2020 18:13
@justinkambic
Copy link
Contributor Author

One successful run of functional tests on the flaky test runner: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/651/

@justinkambic
Copy link
Contributor Author

Second successful flaky test run: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/657/

@justinkambic justinkambic marked this pull request as ready for review July 23, 2020 21:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic added release_note:skip Skip the PR/issue when compiling release notes blocker labels Jul 23, 2020
@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic
Copy link
Contributor Author

A third 100x flaky test run: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/664/

There were failures, but not these tests.

@@ -17,7 +17,7 @@ export function UptimeNavigationProvider({ getService, getPageObjects }: FtrProv
if (await testSubjects.exists('uptimeSettingsToOverviewLink', { timeout: 0 })) {
await testSubjects.click('uptimeSettingsToOverviewLink');
await testSubjects.existOrFail('uptimeOverviewPage', { timeout: 2000 });
} else if (!(await testSubjects.exists('uptimeOverviewPage', { timeout: 0 }))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this isn't related to the alert test flakiness, it's related to the EuiLink not being able to click.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty sure when I was testing this, skipping the contents of this block were causing the test to fail. I can push a commit that reverts this change to see if it keeps succeeding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shahzad31 I am going to need to wait a bit to test this because Jenkins maintenance is happening right now.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

I don't see any issues, but I'm deferring to @shahzad31 for a review of the actual fix

@justinkambic justinkambic requested review from a team as code owners July 27, 2020 18:39
@justinkambic justinkambic requested a review from a team July 27, 2020 18:39
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jul 27, 2020
@elasticmachine
Copy link
Contributor

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

@justinkambic
Copy link
Contributor Author

justinkambic commented Jul 28, 2020

@shahzad31 looking at the results the most recent flaky test run, vs the one I ran yesterday, I'd say that re-introducing the code in that else if condition contributed to the flakiness. If it's an optimization to save time I'm fine with refactoring it but my present goal is to get this test running again as it's a blocker.

We could refactor that code in a follow-up and re-test it but right now I'd like to move on from this issue.

@shahzad31
Copy link
Contributor

@shahzad31 looking at the results the most recent flaky test run, vs the one I ran yesterday, I'd say that re-introducing the code in that else if condition contributed to the flakiness. If it's an optimization to save time I'm fine with refactoring it but my present goal is to get this test running again as it's a blocker.

We could refactor that code in a follow-up and re-test it but right now I'd like to move on from this issue.

@justinkambic sure makes sense. The only part i am concerned about is that, does this also solves the flakiness where we can't click links in alert popover ? i might be wrong, but i thought alert flakiness was bcoz of that.

image

@justinkambic
Copy link
Contributor Author

The only part i am concerned about is that, does this also solves the flakiness where we can't click links in alert popover ? i might be wrong, but i thought alert flakiness was bcoz of that.

@shahzad31 that's something I was concerned over as well but I am not seeing that problem recur at all. I'm wondering if there was some EUI issue that has been subsequently solved. I will run locally and see if I can make that happen again by running the test many times and reply back here.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM, i am also not able to reproduce that unclickable eui link state.

@justinkambic
Copy link
Contributor Author

I am also not able to reproduce that unclickable eui link state.

I'm going to try to smoke test it now as a best effort to make sure we don't knowingly commit a buggy solution, but I am expecting the tests to pass.

@justinkambic
Copy link
Contributor Author

This PR is blocked by #73527. Once a fix is merged, we'll be ready to merge this. And once this unskip is merged, bugs like the one fixed in #73528 will be less likely.

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@justinkambic
Copy link
Contributor Author

I ran a final flaky test check and this passed. I think we are good to merge now. https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/718/

@justinkambic justinkambic merged commit df3e209 into elastic:master Jul 31, 2020
justinkambic added a commit to justinkambic/kibana that referenced this pull request Jul 31, 2020
* Unskip monitor status alert test.

* Trying to resolve flakiness.

* Remove commented code.

* Simplify test expect.

* Revert conditional block change.

* Remove line in question.

Co-authored-by: Elastic Machine <[email protected]>
@justinkambic justinkambic deleted the uptime_unskip-alerting-tests branch July 31, 2020 15:03
justinkambic added a commit that referenced this pull request Jul 31, 2020
* Unskip monitor status alert test.

* Trying to resolve flakiness.

* Remove commented code.

* Simplify test expect.

* Revert conditional block change.

* Remove line in question.

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@justinkambic
Copy link
Contributor Author

justinkambic commented Jul 31, 2020

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 4, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

2 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

justinkambic added a commit that referenced this pull request Aug 6, 2020
* Unskip monitor status alert test.

* Trying to resolve flakiness.

* Remove commented code.

* Simplify test expect.

* Revert conditional block change.

* Remove line in question.

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability test_xpack_functional v7.9.0 v7.10.0 v8.0.0
Projects
None yet
5 participants