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

Paramaterized Discover tests #129684

Merged
merged 11 commits into from
Apr 13, 2022
Merged

Paramaterized Discover tests #129684

merged 11 commits into from
Apr 13, 2022

Conversation

cuff-links
Copy link
Contributor

Part of #126392

@cuff-links cuff-links requested a review from a team as a code owner April 6, 2022 23:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-qa (Team:QA)

@cuff-links cuff-links added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 6, 2022
dmlemeshko
dmlemeshko reviewed Apr 7, 2022
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

LGTM, just would like to not have if/else in before hook :)

test/functional/apps/discover/_data_view_editor.ts Outdated Show resolved Hide resolved
@cuff-links cuff-links requested review from spalger and dmlemeshko April 8, 2022 05:35
kertal
kertal reviewed Apr 12, 2022
View reviewed changes
test/functional/apps/discover/index.ts Outdated
Comment on lines 26 to 28
if (config.get('esTestCluster.ccs')) {
loadTestFile(require.resolve('./_data_view_editor'));
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Is this check necessary? because I think you've prepared _data_view_editor .ts to work with CCS and non-CCS configuration, but that if check would only run it with CCS config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check is necessary. The test is run in both configs at line 61, this same file is run if the config is not CCS. When CCS is enabled, we only want the CCS tests to run. I did, however, miss the saved_queries test so I will add that to work when ccs is enabled and push.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yes I missed that, thanks for the clarification ! 👍

@kertal kertal self-requested a review April 12, 2022 16:25
kertal
kertal approved these changes Apr 12, 2022
View reviewed changes
Copy link
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.

Approving it, given CI turns green. Thx a lot for cleaning this up, this is a better approach to run a test in 2 environments without duplicating code 👍

@cuff-links
Copy link
Contributor Author

I appreciate your feedback. There was a failure but I know what happened. I will fix and push. We need to add the same conditional for the index pattern when in ccs mode. Thanks again for taking the time to review.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@cuff-links cuff-links merged commit fbd38a7 into elastic:main Apr 13, 2022
kibanamachine pushed a commit that referenced this pull request Apr 13, 2022
Paramaterized Discover tests (#129684)
a14a2ae 
* Migrated Discover visualize tests to use CCS.

* Fixed.

* Forgot to add the config file to the functional tests command,

* Fixed nits in PR.

* Removed unused references.

* Removed unused references.

* Removed test file that was removed in the merge.

* Added saved_queries test to run when CCS is enabled and put test that wasn't supposed to be removed back.

* Used conditional in index pattern.

(cherry picked from commit fbd38a7)
@kibanamachine kibanamachine mentioned this pull request Apr 13, 2022
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.2

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 13, 2022
@kibanamachine
Paramaterized Discover tests (#129684) (#130075)
2ea6dc8 
* Migrated Discover visualize tests to use CCS.

* Fixed.

* Forgot to add the config file to the functional tests command,

* Fixed nits in PR.

* Removed unused references.

* Removed unused references.

* Removed test file that was removed in the merge.

* Added saved_queries test to run when CCS is enabled and put test that wasn't supposed to be removed back.

* Used conditional in index pattern.

(cherry picked from commit fbd38a7)

Co-authored-by: John Dorlus <silne.dorlus@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/soft-…
26b64f9 
…disable-server-side

* 'main' of github.com:elastic/kibana: (35 commits)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  [Security Solution] More Ransomware exceptionable fields (elastic#130039)
  Add e2e for the apm integration policy form (elastic#129860)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)
  [ML] Fix Single Metric Viewer chart failing to load if no points during calendar event (elastic#130000)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/screenshots/index.test.ts
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/poc-p…
d49e398 
…rint-media-attempt-2

* 'main' of github.com:elastic/kibana: (75 commits)
  [Lens] Hide disabled toolbar entries (elastic#129994)
  Fix explore tables don't display data when a global filter is applied (elastic#130024)
  [Console] Add option to disable keyboard shortcuts (elastic#128887)
  [Discover] Update refreshOnClick flaky test (elastic#130001)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/formats/pdf/index.ts
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

@spalger spalger spalger approved these changes

@LeeDr LeeDr Awaiting requested review from LeeDr

@wayneseymour wayneseymour Awaiting requested review from wayneseymour

@dmlemeshko dmlemeshko Awaiting requested review from dmlemeshko

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application Feature:Functional Testing release_note:skip Skip the PR/issue when compiling release notes Team:QA Team label for QA Team test_ui_functional test_xpack_functional v8.2.0 v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cuff-links @elasticmachine @kibana-ci @kibanamachine @kertal @spalger @dmlemeshko @LeeDr