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

Fix failing applications navigation test #168302

Merged
merged 24 commits into from
Oct 17, 2023

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Oct 6, 2023

fix #166677

@TinaHeiligers TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc 8.12 candidate release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.12.0 and removed 8.12 candidate labels Oct 6, 2023
@TinaHeiligers
Copy link
Contributor Author

verifying test stability: flaky test run build 3402

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers marked this pull request as ready for review October 9, 2023 15:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

TLDR it does not look like a matter of timeouts to me, I think we will need to try to understand why the URL fails to change sometimes.

return await retry.waitForWithTimeout('navigates to app root', time ?? 3000, async () => {
return (await browser.getCurrentUrl()) === expectedUrl;
});
};
/** Use retry logic to make URL assertions less flaky */
const waitForUrlToBe = (pathname?: string, search?: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing method relies on retry.waitFor.
I've been digging into the code, and found out that the waitFor implementation calls the waitForWithTimeout too.
It does so using timeouts.waitFor timeout config, which defaults to 20 seconds.

It's too bad that the CI artifacts collected upon failure can't show the browser URL bar, but I think that if this URL hasn't changed after 20 seconds, it likely won't change, so the problem must be 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.

Thanks for the tips, I'll look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gsoldevila it looks like most of the ftr tests not in x-pack that implement appsMenu.clickLink(title) directly are skipped, probably because they're failing.

The Dashboard tests aren't skipped though, so I implemented something similar.

Problem is, we still don't know what causes these failures and need to figure that out.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers marked this pull request as draft October 11, 2023 15:46
@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Oct 11, 2023

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers marked this pull request as ready for review October 12, 2023 02:29
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

I agree with @gsoldevila that I'm not sure this is going to solve anything given in the end, we're going the exact (just adding an additional wait gate via waitUntilLoadingIsDone)

However, we never know, and I don't see harm merging this to see if that solve the flakiness, so LGTM.

My 2 cps though:

The current error surfaced in #166677 is.

Error: timed out waiting for Url to be http://localhost:5620/app/foo/home

Can we at least modify this test's assertion so that when it fail, it displays the actual url so that we at least have the slightest idea of the problem the next time it fails? That's the only way I see to be able to really progress in this issue.

test/plugin_functional/README.md Outdated Show resolved Hide resolved
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers requested review from a team as code owners October 13, 2023 23:37
@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -80,8 +80,8 @@ export default function ({
await testSubjects.missingOrFail('superDatePickerToggleQuickMenuButton');
await testSubjects.existOrFail('globalQueryBar');
});

it('renders as expected', async () => {
// failed test. See https://github.com/elastic/kibana/issues/163207
Copy link
Contributor

Choose a reason for hiding this comment

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

This outer test suite (default URL params) was already skipped - is there a reason this inner test was also skipped? :)

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) October 16, 2023 19:41
@TinaHeiligers TinaHeiligers enabled auto-merge (squash) October 16, 2023 21:02
@TinaHeiligers TinaHeiligers dismissed gsoldevila’s stale review October 16, 2023 23:16

We'll investigate deeper once we have the logs

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #75 / AIOps POST /internal/aiops/log_rate_analysis - groups only with ecommerce should return group only in chunks with streaming without compression without flushFix

Metrics [docs]

✅ unchanged

History

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

@TinaHeiligers TinaHeiligers merged commit ac80b2a into elastic:main Oct 17, 2023
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
fix [elastic#166677 ](elastic#166677) 

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@gsoldevila
Copy link
Contributor

Can we at least modify this test's assertion so that when it fail, it displays the actual url so that we at least have the slightest idea of the problem the next time it fails? That's the only way I see to be able to really progress in this issue.

FWIW we do have this information in the traces, that's how I found out that navigation was not taking place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.12.0
Projects
None yet
7 participants