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

Functional tests: refactor visualize_page #53845

Merged
merged 17 commits into from
Jan 7, 2020

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Dec 30, 2019

Summary

This PR aim is to improve the developer's experience in writing functional tests. As described in #53117, visualize_page is one of those "heavy" PO: with 1.5k lines and ~200 functions, it is hard to search and determine suitable function.

The idea is to split visualize_page into smaller POs that represents different areas, remove duplicate functions, combine similar actions and convert it to TS:

  • visualize_page: interactions on the landing page (open, save, create new viz)
  • visualize_editor_page: interactions related to editor panel
  • visualize_chart_page: common interactions with chart viz and legend
  • tile_map_page: tilemap viz specific interactions
  • tag_cloud_page: tag cloud specific interactions
  • vega_chart_page: vega viz specific interactions

PO2

PO1

@kibanamachine
Copy link
Contributor

💔 Build Failed

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

@kibanamachine
Copy link
Contributor

💔 Build Failed

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

@dmlemeshko
Copy link
Member Author

@elasticmachine merge upstream

@dmlemeshko
Copy link
Member Author

@elasticmachine merge upstream

@@ -19,13 +19,25 @@

import { FtrProviderContext } from '../ftr_provider_context';

export function VisualizeListingTableProvider({ getService, getPageObjects }: FtrProviderContext) {
export function ListingTableProvider({ getService, getPageObjects }: FtrProviderContext) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can be reused for Dashboard and Settings landing pages

@dmlemeshko dmlemeshko marked this pull request as ready for review January 3, 2020 20:44
@dmlemeshko dmlemeshko added the release_note:skip Skip the PR/issue when compiling release notes label Jan 3, 2020
@dmlemeshko dmlemeshko requested review from sulemanof, spalger and a team January 3, 2020 20:46
@dmlemeshko
Copy link
Member Author

@elasticmachine merge upstream

await testSubjects.existOrFail('visLibVisualizeError');
}

public async getChartTypes() {
Copy link
Contributor

@LeeDr LeeDr Jan 6, 2020

Choose a reason for hiding this comment

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

I think getChartTypes should be in visualize_page?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, fixing it

const listingTable = getService('listingTable');
const { common, header, visEditor } = getPageObjects(['common', 'header', 'visEditor']);

class VisualizePage {
Copy link
Contributor

@LeeDr LeeDr Jan 6, 2020

Choose a reason for hiding this comment

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

Maybe we can have a comment block here describing that this page object file contains the visualization type selection, the landing page, and the open/save dialog functions (is that correct?)

I was going to suggest we rename it to visualization_types_page or something like that, but now I see it has more than that.

Copy link
Contributor

@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 - a few small comments you can address if you like. Wow, this looks like it was a lot of work but really makes it better! Some of the visualization_chart_page will be obsolete as we move over to elastic-charts library. But it's still a great improvement.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@dmlemeshko dmlemeshko merged commit a1176b0 into elastic:master Jan 7, 2020
dmlemeshko added a commit to dmlemeshko/kibana that referenced this pull request Jan 7, 2020
* add new POs and services

* split visualize_page

* refactor PO and tests

* lost changes

* more fixes

* fix tslint error

* refactor POs

* add vega_chart_page, refactor

* review fix

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
dmlemeshko added a commit that referenced this pull request Jan 7, 2020
* add new POs and services

* split visualize_page

* refactor PO and tests

* lost changes

* more fixes

* fix tslint error

* refactor POs

* add vega_chart_page, refactor

* review fix

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 7, 2020
…le-saved-objects

* 'master' of github.com:elastic/kibana: (86 commits)
  [Uptime] Added date range filter into expanded list query (elastic#52609)
  [SIEM] Add react/display-name eslint rule (elastic#53107)
  [SIEM] Enable eslint prefer-template rule (elastic#53983)
  Elasticsearch snapshots automation (elastic#53706)
  [SIEM] Enable eslint react/no-children-prop (elastic#53985)
  [DOCS][Reporting] Adds info about using a custom reporting index (elastic#54052)
  Changing background color to align with EUI color (elastic#54060)
  Fix linting issues (elastic#54068)
  NP Migration: Move doc views registry and existing doc views into discover plugin (elastic#53465)
  [ML] DF Analytics job creation: Add 'excludes' input field to form (elastic#53856)
  EMT-issue-65: add endpoint list api (elastic#53861)
  [SIEM] Fix doubled drag handles in Timeline (elastic#52679)
  Functional tests: refactor visualize_page (elastic#53845)
  [Dashboard] Redesign empty screen in readonly mode (elastic#54073)
  [ML] Support search for partitions on Single Metric Viewer (elastic#53879)
  update apm index pattern (elastic#54095)
  Add data ui for envoyproxy Metricbeat Module (elastic#53476)
  [ML] persist the brush when expanded to full width (elastic#54020)
  Skip failing test (elastic#54100)
  [APM] Show errors on the timeline instead of under the transaction (elastic#53756)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/np_ready/application/contexts/services_context.tsx
#	src/legacy/core_plugins/console/public/np_ready/application/index.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 7, 2020
…/kibana into feature/console-saved-objects

* 'feature/console-saved-objects' of github.com:jloleysens/kibana: (86 commits)
  [Uptime] Added date range filter into expanded list query (elastic#52609)
  [SIEM] Add react/display-name eslint rule (elastic#53107)
  [SIEM] Enable eslint prefer-template rule (elastic#53983)
  Elasticsearch snapshots automation (elastic#53706)
  [SIEM] Enable eslint react/no-children-prop (elastic#53985)
  [DOCS][Reporting] Adds info about using a custom reporting index (elastic#54052)
  Changing background color to align with EUI color (elastic#54060)
  Fix linting issues (elastic#54068)
  NP Migration: Move doc views registry and existing doc views into discover plugin (elastic#53465)
  [ML] DF Analytics job creation: Add 'excludes' input field to form (elastic#53856)
  EMT-issue-65: add endpoint list api (elastic#53861)
  [SIEM] Fix doubled drag handles in Timeline (elastic#52679)
  Functional tests: refactor visualize_page (elastic#53845)
  [Dashboard] Redesign empty screen in readonly mode (elastic#54073)
  [ML] Support search for partitions on Single Metric Viewer (elastic#53879)
  update apm index pattern (elastic#54095)
  Add data ui for envoyproxy Metricbeat Module (elastic#53476)
  [ML] persist the brush when expanded to full width (elastic#54020)
  Skip failing test (elastic#54100)
  [APM] Show errors on the timeline instead of under the transaction (elastic#53756)
  ...
@dmlemeshko dmlemeshko deleted the ftr/reorganize--page-objects branch January 31, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants