Skip to content

Convert discover_page to ts, remove redundunt methods#59312

Merged
dmlemeshko merged 3 commits intoelastic:masterfrom
dmlemeshko:ftr/tsfy-page-objects
Mar 5, 2020
Merged

Convert discover_page to ts, remove redundunt methods#59312
dmlemeshko merged 3 commits intoelastic:masterfrom
dmlemeshko:ftr/tsfy-page-objects

Conversation

@dmlemeshko
Copy link
Copy Markdown
Contributor

@dmlemeshko dmlemeshko commented Mar 4, 2020

Summary

Convert discover_page to ts, remove redundant/duplicate methods

@dmlemeshko dmlemeshko requested a review from a team March 4, 2020 14:51
@dmlemeshko dmlemeshko added test_ui_functional v7.7.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 4, 2020
LeeDr
LeeDr approved these changes Mar 4, 2020
View reviewed changes
Copy link
Copy Markdown

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM

@dmlemeshko dmlemeshko marked this pull request as ready for review March 4, 2020 15:49
@dmlemeshko dmlemeshko requested a review from a team March 4, 2020 15:49
@timroes timroes requested a review from kertal March 4, 2020 16:02
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 4, 2020

💚 Build Succeeded

History

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

kertal
kertal reviewed Mar 5, 2020
View reviewed changes
test/functional/page_objects/discover_page.ts
const $ = await table.parseDomContent();
return $('mark')
.toArray()
.map(mark => $(mark).text());
Copy link
Copy Markdown
Member

@kertal kertal Mar 5, 2020

Choose a reason for hiding this comment

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

What's the reason for the switch to parseDomContent change? code becomes more verbose? thx!

Copy link
Copy Markdown
Contributor Author

@dmlemeshko dmlemeshko Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

To speedup and minimize flakiness:

      const marks = await find.allByCssSelector('mark');
      return await Promise.all(marks.map(mark => mark.getVisibleText()));

With the code above _field_data test webdriver makes 51 rest call (1 - /elements, 50 for each element) The main problem is that while iterating through array of 50 elements DOM might be updated and mark.getVisibleText() will fail since element reference is no longer valid.

With the new code we make 2 rest calls (1 - /element, 1 - /innerHTML), faster and since we search in DOM snapshot there is no longer dependency UI rendering.

Copy link
Copy Markdown
Contributor Author

@dmlemeshko dmlemeshko Mar 5, 2020

Choose a reason for hiding this comment

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

it is not the best approach for every case, but here mark is in the span and I don't see a reason why taking text from DOM can be different from text we are getting with webdriver rest call.

Copy link
Copy Markdown
Member

@kertal kertal Mar 5, 2020

Choose a reason for hiding this comment

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

thank you very much for the explanation!

@kertal kertal self-requested a review March 5, 2020 12:28
kertal
kertal approved these changes Mar 5, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, reducing the number of requests and improving stability is a nice improvement. didn't test, but this is done by Lord Jenkins anyway

@dmlemeshko dmlemeshko merged commit c42bd33 into elastic:master Mar 5, 2020
@dmlemeshko dmlemeshko mentioned this pull request Mar 5, 2020
Merged
dmlemeshko added a commit to dmlemeshko/kibana that referenced this pull request Mar 5, 2020
@dmlemeshko
Convert discover_page to ts, remove redundunt methods (elastic#59312)
ebc1f20 
* convert discover_page to ts

* remove deuplicated methods, improve tests
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 5, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into console/featu…
ce71db9 
…re/files-and-filetree

* 'master' of github.com:elastic/kibana: (254 commits)
  Convert discover_page to ts, remove redundunt methods (elastic#59312)
  [Fix for Vis Editor] Revert setting time field to empty string when it's undefined (elastic#58873)
  Delete legacy search endpoint (elastic#59341)
  [Uptime] Improve duration chart (elastic#58404)
  [Snapshot & Restore] NP migration (elastic#59109)
  [ML] Add support for date_nanos time field in anomaly job wizard (elastic#59017)
  Revert "Makes alerting and actions optional properties for interface RequestH… (elastic#59264)"
  Change remote_clusters ID to remoteClusters (elastic#59246)
  Makes alerting and actions optional properties for interface RequestH… (elastic#59264)
  Clean up date histogram agg type. (elastic#58805)
  [ML] Management: fix license unsubscribe (elastic#59365)
  Remove documentation for server.cors settings (elastic#59096)
  Edit alert flyout (elastic#58964)
  [SIEM] Fix rule delete/duplicate actions (elastic#59306)
  move mouse to close obstructing tooltip (elastic#59214)
  Reset page after deleting (elastic#59310)
  Make sure phrases input filter triggers autosuggestons (elastic#59299)
  Add loading count source for http requests (elastic#59245)
  Revert "[ML] Transforms: Deprecate custom KibanaContext. (elastic#59133)"
  Expose metrics service to public API (elastic#59294)
  ...

# Conflicts:
#	src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
dmlemeshko added a commit to dmlemeshko/kibana that referenced this pull request Mar 5, 2020
@dmlemeshko
Convert discover_page to ts, remove redundunt methods (elastic#59312)
97c7a98 
* convert discover_page to ts

* remove deuplicated methods, improve tests
@dmlemeshko dmlemeshko mentioned this pull request Mar 5, 2020
Merged
dmlemeshko added a commit that referenced this pull request Mar 5, 2020
@dmlemeshko
Convert discover_page to ts, remove redundunt methods (#59312) (#59426)
a4eeee9 
* convert discover_page to ts

* remove deuplicated methods, improve tests
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 6, 2020

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 kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 6, 2020
dmlemeshko added a commit that referenced this pull request Mar 6, 2020
@dmlemeshko @elasticmachine
Convert discover_page to ts, remove redundunt methods (#59312) (#59414)
d0af5e4 
* convert discover_page to ts

* remove deuplicated methods, improve tests

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 6, 2020
@dmlemeshko dmlemeshko deleted the ftr/tsfy-page-objects branch January 31, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kertal kertal kertal approved these changes

+1 more reviewer

@LeeDr LeeDr LeeDr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v7.6.2 v7.7.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dmlemeshko @kibanamachine @kertal @LeeDr