[Fix] Additional tests and clean up for share modal redesign initiative #183643

rshen91 · 2024-05-16T13:40:44Z

Summary

Closes #181066
Closes epic https://github.com/elastic/kibana-team/issues/735
Closes #183752

Based on PR [Tests] Share Modal Redesign clean up and tests #180406, the test 'generates a report with single timefilter' was skipped in x-pack/test/functional/apps/discover/reporting.ts because short URLs are the default and make the test fail.
In addition test/functional/apps/dashboard/group5/share.ts was skipped for similar reasons
x-pack/test/functional/apps/lens/group4/share.ts 'should preserve filter and query when sharing'
x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts refactoring
x-pack/test/functional/apps/discover/feature_controls/discover_security.ts
Add lens by-value FTR to confirm share link in lens is passing a share link that will open to a Lens visualization. For reference check out [Tests] Share Modal Redesign clean up and tests #180406 (review)

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

rshen91 · 2024-05-16T14:36:52Z

/ci

rshen91 · 2024-05-16T18:52:55Z

/ci

elasticmachine · 2024-05-17T00:13:17Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

rshen91 · 2024-05-20T15:46:23Z

@elastic/kibana-operations or @elastic/kibana-qa Lens app - group 4 show underlying data should show the open button for a compatible saved visualization is passing for me locally. Any ideas what I might be missing? Thank you in advance for the help 🙇🏻

Heenawter · 2024-05-21T16:11:02Z

@rshen91 It looks like the default short URL is causing some flakiness with this test: #183752

Running it locally, you can see that, when clicking copy link, the data-share-url parameter defaults to the long URL (including all state) and then gets overwritten with the short URL once it is generated (you can see this param cycle from empty, to the long url, to the short URL in the following video):

Screen.Recording.2024-05-21.at.10.08.55.AM.mov

This is causing flakiness because sometimes the test is fetching it in time to get the long URL, but other times it's too slow and it grabs the short URL instead. In this case, this test is specifically expecting the long URL. Do you mind taking a look into this test here as well? 🙇 Thanks so much!!

dej611

I've been testing various scenarios here and catched a bug.
Will list of the tested scenarios:

Create a visualization via Visualize library
- ✅ Create a chart in Lens and share it before saving
- ✅ Create a chart in Lens, save the chart in library then share it
- ❌ Create a chart in Lens, save the chart in a new dashboard, then edit the panel and share it
  - the URL leads to an empty editor
Create a visualization via Dashboard
- ✅ Create a chart in Lens and share it before saving
- ✅ Create a chart in Lens and click on Save in library, then share it
- ❌ Create a chart in Lens, save and return to dashboard, then edit the panel again and share it
  - the URL leads to an empty editor

I think the problem root of both scenarios are the same. Mind that in that case the shared chart is not a SO, rather a snapshot with short URL.

rshen91 · 2024-05-28T14:08:10Z

I've been testing various scenarios here and catched a bug. Will list of the tested scenarios:

Create a visualization via Visualize library

✅ Create a chart in Lens and share it before saving

✅ Create a chart in Lens, save the chart in library then share it

❌ Create a chart in Lens, save the chart in a new dashboard, then edit the panel and share it

the URL leads to an empty editor

Create a visualization via Dashboard

✅ Create a chart in Lens and share it before saving

✅ Create a chart in Lens and click on Save in library, then share it

❌ Create a chart in Lens, save and return to dashboard, then edit the panel again and share it

the URL leads to an empty editor

I think the problem root of both scenarios are the same. Mind that in that case the shared chart is not a SO, rather a snapshot with short URL.

@dej611 Can lens make these into saved objects vs the snapshot url? I think this is the expected behavior with snapshot urls (which is why we were discussing only saved object urls at one point). I don't think this is something that the share modal can really control for on our side. Thanks

Heenawter

Unskipping of Dashboard tests LGTM 🙇 I am running the two test suites through the FTR just to make sure they aren't introducing flakiness, but still approving to unblock. Thanks so much!!

kibanamachine · 2024-05-29T23:08:54Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6166

[✅] test/functional/apps/dashboard/group3/config.ts: 50/50 tests passed.
[✅] test/functional/apps/dashboard/group5/config.ts: 50/50 tests passed.

see run history

tsullivan

Just left a few comments, but overall this is really great!

test/functional/apps/discover/group5/_shared_links.ts

davismcphee

Looking good overall, just left a few questions about the Data Discovery test changes.

Also I see that discover_security.ts is checked off in the summary list, but it doesn't look like these tests have been restored yet.

davismcphee · 2024-05-30T22:32:03Z

test/functional/apps/discover/group5/_shared_links.ts

I'm a bit confused about the changes here. In #180406 some of the tests in this file were removed because the new share modal defaulted to short URLs, but is that no longer the case?

Good question - we are setting the data-share-url param to be long urls for their tests but yes they're going to be short urls for users.

davismcphee · 2024-05-30T22:36:57Z

x-pack/test/functional/apps/discover/reporting.ts

@@ -103,8 +103,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
        await PageObjects.discover.selectIndexPattern('ecommerce');
      });

-      // Discover defaults to short urls - is this test helpful? Clarify in separate PR
-      xit('generates a report with single timefilter', async () => {
+      it('generates a report with single timefilter', async () => {


It looks like the serverless version of this test in x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts hasn't been unskipped yet.

Also there were some other minor changes to these files in #180406, like removing gte:now-24h/h,lte:now)))) from the reportURL check, and removing await PageObjects.reporting.checkForReportingToasts() between tests. Can any of these be restored, or is the behaviour different now?

Looks like this question has been partially resolved: the serverless test isn't skipped.

I also wonder about the minor change, and wonder if we can restore the validation of the sharing URL that @davismcphee wrote about:

diff --git x-pack/test/functional/apps/discover/reporting.ts x-pack/test/functional/apps/discover/reporting.ts index 0b193e5b850..ab5ee99448b 100644 --- x-pack/test/functional/apps/discover/reporting.ts +++ x-pack/test/functional/apps/discover/reporting.ts @@ -130,7 +130,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(timeFiltersNumberInReportURL).to.be(1); expect( decodeURIComponent(reportURL).includes( - 'query:(range:(order_date:(format:strict_date_optional_time' + 'query:(range:(order_date:(format:strict_date_optional_time,gte:now-24h/h,lte:now))))' ) ).to.be(true); diff --git x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts index c0e99978750..1a500817ddd 100644 --- x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts +++ x-pack/test_serverless/functional/test_suites/common/discover/x_pack/reporting.ts @@ -143,7 +143,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(timeFiltersNumberInReportURL).to.be(1); expect( decodeURIComponent(reportURL).includes( - 'query:(range:(order_date:(format:strict_date_optional_time' + 'query:(range:(order_date:(format:strict_date_optional_time,gte:now-24h/h,lte:now))))' ) ).to.be(true);

Thank you for pointing this out @davismcphee! Turns out you found a potential issue and now is fixed in 510d054. The tests are restored to show the absolute time of now etc. as it should be for watcher. Thanks!!

…-redesign

tsullivan

LGTM, great work!

kibana-ci · 2024-06-06T15:04:31Z

💚 Build Succeeded

Buildkite Build
Commit: da8b0a9

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.5MB	1.5MB	+56.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`reporting`	52.8KB	52.8KB	+2.0B
`share`	55.9KB	56.0KB	+75.0B
total			+77.0B

History

💛 Build #214158 was flaky 74916ea
💚 Build #213985 succeeded 84de2d6
💛 Build #213939 was flaky 510d054
💚 Build #213737 succeeded 01cf9d9
💚 Build #213672 succeeded 467d033
💔 Build #213575 failed ae26bba

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

cc @rshen91

rshen91 self-assigned this May 16, 2024

rshen91 added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label May 16, 2024

rshen91 marked this pull request as ready for review May 17, 2024 00:13

rshen91 requested review from a team as code owners May 17, 2024 00:13

rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label May 17, 2024

kibanamachine requested a review from a team as a code owner May 17, 2024 15:41

rshen91 removed the request for review from a team May 17, 2024 15:50

dej611 requested changes May 24, 2024

View reviewed changes

rshen91 requested a review from dej611 May 28, 2024 16:54

Heenawter approved these changes May 29, 2024

View reviewed changes

rshen91 added 10 commits May 30, 2024 07:30

fix discover test

7476711

remove comment

b2f403b

remove skips

1619d30

wip new lens test

80099be

fix lens test

9d00084

fix start of test

c41f488

doc readme addition

fa3ebb5

only long url for share-url param

a821d72

fix data-share-url

bee37d9

fix test

afe3f23

tsullivan reviewed May 30, 2024

View reviewed changes

test/functional/apps/discover/group5/_shared_links.ts Outdated Show resolved Hide resolved

test/functional/apps/discover/group5/_shared_links.ts Show resolved Hide resolved

rshen91 added 3 commits May 30, 2024 14:56

remove listing table

d10a491

Merge remote-tracking branch 'upstream/main' into clean-up-redesign

2b881e0

code review

9e2400c

rshen91 requested a review from tsullivan May 30, 2024 21:25

davismcphee reviewed May 30, 2024

View reviewed changes

rshen91 added 15 commits May 30, 2024 19:29

Merge remote-tracking branch 'upstream/main' into clean-up-redesign

52b86c6

save vis

7f6153f

Merge remote-tracking branch 'upstream/main' into clean-up-redesign

c379c08

unskip test

a542529

update

c7a72b7

remove

2ba2d10

restore snapshots

f3adfc7

Merge branch 'main' into clean-up-redesign

987a4ae

Merge branch 'main' into clean-up-redesign

0a023e7

Merge branch 'main' into clean-up-redesign

ae26bba

Merge remote-tracking branch 'upstream/main' into clean-up-redesign

5183f26

Merge remote-tracking branch 'origin/clean-up-redesign' into clean-up…

6750f6c

…-redesign

Merge branch 'main' into clean-up-redesign

467d033

update test for better accuracy

b4d7af5

Merge remote-tracking branch 'origin/clean-up-redesign' into clean-up…

01cf9d9

…-redesign

dej611 approved these changes Jun 4, 2024

View reviewed changes

fix absolute for watcher share url

510d054

rshen91 requested a review from davismcphee June 4, 2024 16:43

tsullivan approved these changes Jun 4, 2024

View reviewed changes

Merge branch 'main' into clean-up-redesign

84de2d6

rshen91 enabled auto-merge (squash) June 4, 2024 18:52

rshen91 added 2 commits June 5, 2024 08:09

Merge branch 'main' into clean-up-redesign

74916ea

Merge branch 'main' into clean-up-redesign

da8b0a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Additional tests and clean up for share modal redesign initiative #183643

[Fix] Additional tests and clean up for share modal redesign initiative #183643

rshen91 commented May 16, 2024 •

edited

rshen91 commented May 16, 2024

rshen91 commented May 16, 2024

elasticmachine commented May 17, 2024

rshen91 commented May 20, 2024 •

edited

Heenawter commented May 21, 2024 •

edited

dej611 left a comment

rshen91 commented May 28, 2024

Heenawter left a comment •

edited

kibanamachine commented May 29, 2024

tsullivan left a comment

davismcphee left a comment

davismcphee May 30, 2024

rshen91 May 31, 2024

davismcphee May 30, 2024

tsullivan Jun 3, 2024

rshen91 Jun 4, 2024

tsullivan left a comment

kibana-ci commented Jun 6, 2024

[Fix] Additional tests and clean up for share modal redesign initiative #183643

Are you sure you want to change the base?

[Fix] Additional tests and clean up for share modal redesign initiative #183643

Conversation

rshen91 commented May 16, 2024 • edited

Summary

Checklist

rshen91 commented May 16, 2024

rshen91 commented May 16, 2024

elasticmachine commented May 17, 2024

rshen91 commented May 20, 2024 • edited

Heenawter commented May 21, 2024 • edited

dej611 left a comment

Choose a reason for hiding this comment

rshen91 commented May 28, 2024

Heenawter left a comment • edited

Choose a reason for hiding this comment

kibanamachine commented May 29, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#6166

tsullivan left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee May 30, 2024

Choose a reason for hiding this comment

rshen91 May 31, 2024

Choose a reason for hiding this comment

davismcphee May 30, 2024

Choose a reason for hiding this comment

tsullivan Jun 3, 2024

Choose a reason for hiding this comment

rshen91 Jun 4, 2024

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 6, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

rshen91 commented May 16, 2024 •

edited

rshen91 commented May 20, 2024 •

edited

Heenawter commented May 21, 2024 •

edited

Heenawter left a comment •

edited