[Dashboard Navigation] Unskip tests #211660
Conversation
Heenawter
added
Team:Presentation
| // TODO We should not be using an external website for our tests. This will be flaky | ||
| // if external network connectivity issues exist. |
There was a problem hiding this comment.
@nickpeihl You added this comment in #167896, but there was no further context. Do you recall what network connectivity issues would cause this to be flaky?
We aren't checking the contents of the external URL, we are just ensuring that navigation happens as expected (i.e. verifying that the browser's URL matches the external link URL). I've run this through the FTR 200 times and saw no issues with flakiness. https://example.com/ is also fully safe to navigate to, since it is a URL that is reserved for documentation + testing purposes - so I'm having a hard time thinking of any issues we could see here 🤔 Would appreciate your input before we unskip these!
There was a problem hiding this comment.
I don't fully recall if there was a documented reason to avoid using external resources in tests. CI can run in numerous environments which may affect loading external resources. That said, you may be correct that the test would still pass even if https://example.com can not be loaded.
But now I think these functional tests could be removed in favor of the unit tests which are cheaper and test the same features.
There was a problem hiding this comment.
We could definitely replace some of these tests with unit tests! But is it okay if we merge this as-is for now?
There was a problem hiding this comment.
We already have unit tests for opening in a new tab, so I don't think we need to create any new tests. But I'm OK with merging this to unblock.
There was a problem hiding this comment.
Do you mind giving this a quick approval? We can revisit removing these duplicate tests + replacing functional tests with unit tests as part of the Links embeddable test audit 🙇
Heenawter
added
test
v9.0.0
backport:version
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#7902[✅] test/functional/apps/dashboard_elements/links/config.ts: 100/100 tests passed. |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
💚 Build Succeeded
Metrics [docs]
History
cc @Heenawter |
Closes elastic#210618 ## Summary This PR unskips all Links tests that were skipped since these tests are not actually flaky at this point in time. I ran them through the FTR 200 times ([run 1](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7900), [run 2](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7902)) to confirm this. ### Checklist - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Closes #210618
Summary
This PR unskips all Links tests that were skipped since these tests are not actually flaky at this point in time. I ran them through the FTR 200 times (run 1, run 2) to confirm this.
Checklist
release_note:*label is applied per the guidelines