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.

Sign up for GitHub

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

[Dashboard Navigation] Unskip tests #211660

Merged
merged 5 commits into from
Feb 25, 2025
+2 −4
Merged

[Dashboard Navigation] Unskip tests #211660

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8b0c88
Try FTR
Heenawter Feb 18, 2025
dd87308
Remove `.only`
Heenawter Feb 18, 2025
e7d9a24
Merge branch 'main' into unskip-links-tests_2025-02-18
Heenawter Feb 21, 2025
02cc534
Merge branch 'main' into unskip-links-tests_2025-02-18
Heenawter Feb 24, 2025
0084b34
Merge branch 'main' into unskip-links-tests_2025-02-18
Heenawter Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions test/functional/apps/dashboard_elements/links/links_navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
expect(isDisabled).to.be('true');
});

// TODO We should not be using an external website for our tests. This will be flaky
// if external network connectivity issues exist.
Comment on lines -217 to -218
Copy link
Contributor Author

@Heenawter Heenawter Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could definitely replace some of these tests with unit tests! But is it okay if we merge this as-is for now?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙇

it.skip('should create an external link when openInNewTab is enabled', async () => {
it('should create an external link when openInNewTab is enabled', async () => {
await testSubjects.clickWhenNotDisabled('externalLink--link999');

// Should have opened another tab
Expand All @@ -227,7 +225,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
expect(currentUrl).to.be('https://example.com/1');
});

it.skip('should open in same tab when openInNewTab is disabled', async () => {
it('should open in same tab when openInNewTab is disabled', async () => {
await testSubjects.clickWhenNotDisabled('externalLink--link888');

// Should have opened in the same tab
Expand Down