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

Jump to bottom

Report Deletion via UI- functional test #64031

Merged
merged 9 commits into from
Apr 24, 2020
Merged

Report Deletion via UI- functional test #64031

merged 9 commits into from
Apr 24, 2020

Conversation

rashmivkulkarni
Copy link
Contributor

@rashmivkulkarni rashmivkulkarni commented Apr 21, 2020 •
edited
Loading

resolves #63296
resolves: #62874 -by adding a pagination test

@rashmivkulkarni rashmivkulkarni added (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead test_xpack_functional v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Functional Testing labels Apr 21, 2020
@rashmivkulkarni rashmivkulkarni self-assigned this Apr 21, 2020
@rashmivkulkarni
Copy link
Contributor Author

@elasticmachine merge upstream

@rashmivkulkarni
Copy link
Contributor Author

jenkins, test this

@rashmivkulkarni rashmivkulkarni marked this pull request as ready for review April 22, 2020 19:38
tsullivan
tsullivan requested changes Apr 22, 2020
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Request to go back to the original navigate solution, instead of wiring up the Reporting management page to be treated like an app. :)

x-pack/test/functional/config.js Show resolved Hide resolved
@rashmivkulkarni
Copy link
Contributor Author

Request to go back to the original navigate solution, instead of wiring up the Reporting management page to be treated like an app. :)

  • Reverted back to the old method of navigating via the URL().

@rashmivkulkarni
Copy link
Contributor Author

@spalger - can you please weigh in on which method would be more appropriate here :navigateToApp or navigateTOActualURL

@rashmivkulkarni
Copy link
Contributor Author

I did talk to Spencer about it. He agrees with @tsullivan on this ."how were we handling this before for this feature" is more about aligning it with other Reporting tests.

@tsullivan
Copy link
Member

tsullivan commented Apr 23, 2020 •
edited
Loading

Thanks @Rasroh. The other Reporting tests verify the app integration. So they'd start with navigateToApp('dashboard'), etc. This is a new test to verify the job listing page in management.

  • The job listing page is a UI of the Reporting Stack Service
  • The app that runs the UI is Management

Earlier I said that it's important to not enforce an idea that Reporting is an app in Kibana. Reporting is the thing that the apps use to enhance their capabilities - that's why we're happier calling it a stack service. We find that when folks think of Reporting as an app, they expect the Reporting team to have some ownership about the PDF/PNG rendering problems due to the apps not being gracefully integrated - those problems have nothing to do with the stack service.

@rashmivkulkarni
Copy link
Contributor Author

Thanks @tsullivan for detailed explanation.

tsullivan
tsullivan approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM!

@rashmivkulkarni
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@LeeDr
Copy link

LeeDr commented Apr 23, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

LeeDr
LeeDr approved these changes Apr 24, 2020
View reviewed changes
Copy link

@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 - I didn't pull this locally. But I code reviewed including checking that the test is run with a user with the minimum role and restoreRoles is called in the after method.

And I verified the new additional test results in Jenkins;

x-pack/test/functional/apps/reporting_management/report_delete_pagination·ts 6.1 sec 0   0   2 +2 2 +2

@rashmivkulkarni rashmivkulkarni merged commit 9703d85 into elastic:master Apr 24, 2020
@rashmivkulkarni rashmivkulkarni mentioned this pull request Apr 24, 2020
Merged
rashmivkulkarni added a commit to rashmivkulkarni/kibana that referenced this pull request Apr 24, 2020
@rashmivkulkarni @elasticmachine
Report Deletion via UI- functional test (elastic#64031)
dfd2743 
* report delete test

* removed the exclusive test

* new archived reports

* added pagination test

* implemented review changes

* Removed the unwanted method

* addressed the review comments

Co-authored-by: Elastic Machine <[email protected]>
rashmivkulkarni added a commit that referenced this pull request Apr 25, 2020
@rashmivkulkarni @elasticmachine
Report Deletion via UI- functional test (#64031) (#64466)
eba00e8 
* report delete test

* removed the exclusive test

* new archived reports

* added pagination test

* implemented review changes

* Removed the unwanted method

* addressed the review comments

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 27, 2020
@jloleysens
Merge branch 'feature/ingest-node-pipelines' of github.com:elastic/ki…
42247e2 
…bana into ingest-node-pipeline/open-flyout-create-edit

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits)
  [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411)
  Report Deletion via UI- functional test (elastic#64031)
  Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402)
  [Uptime] Update TLS settings (elastic#64111)
  [alerting] removes usage of any throughout Alerting Services code (elastic#64161)
  [CANVAS] Moves notify to a canvas service (elastic#63268)
  [Canvas] Misc NP Stuff (elastic#63703)
  update apm index pattern (elastic#64232)
  Task/hostlist pagination (elastic#63722)
  [NP] Vega migration (elastic#63849)
  Move ensureDefaultIndexPattern into data plugin (elastic#63100)
  [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106)
  Migrate graph_workspace saved object registration to Kibana platform (elastic#64157)
  Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184)
  [ML] EuiDataGrid ml/transform components. (elastic#63447)
  [ML] Moving to kibana capabilities (elastic#64057)
  Move input_control_vis into NP (elastic#63333)
  remove reference to local application service in graph (elastic#64288)
  KQL removes leading zero and breaks query (elastic#62748)
  [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeeDr LeeDr LeeDr approved these changes

@tsullivan tsullivan tsullivan approved these changes

@dmlemeshko dmlemeshko Awaiting requested review from dmlemeshko

Assignees

@rashmivkulkarni rashmivkulkarni

Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Feature:Functional Testing release_note:skip Skip the PR/issue when compiling release notes test_xpack_functional v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delete Reports Functional Test Kibana doesn't display more than 10 reports
6 participants
@rashmivkulkarni @tsullivan @LeeDr @kibanamachine @dmlemeshko @elasticmachine