Skip to content

[Discover] Improve indexpattern without timefield functional test#67031

Merged
kertal merged 4 commits intoelastic:masterfrom
kertal:kertal-pr-2020-05-19-discover-improve-indexpattern-without-timefield-test
May 25, 2020
Merged

[Discover] Improve indexpattern without timefield functional test#67031
kertal merged 4 commits intoelastic:masterfrom
kertal:kertal-pr-2020-05-19-discover-improve-indexpattern-without-timefield-test

Conversation

@kertal
Copy link
Copy Markdown
Member

@kertal kertal commented May 19, 2020 •
edited
Loading

Summary

While the flaky test runner couldn't reproduce the flakiness, I've improved the code by pre-selecting the index pattern in the uiSettings. A screenshot of a cloud failure, pointed to this direction. The test was stuck selecting the right index pattern. This simple tweak reduced the time used for this test to about 20% (!) of the former implementation.

Fixes #63273

Here's the flaky test runner run of this implementation: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/470/

Checklist

Delete any items that are not applicable to this PR.

@kertal kertal added the Feature:Discover Discover Application label May 19, 2020
@kertal kertal self-assigned this May 20, 2020
@kertal kertal added :KibanaApp/fix-it-week release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v7.9.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels May 20, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 20, 2020

Pinging @elastic/kibana-app (Team:KibanaApp)

@kertal kertal requested review from liza-mae and spalger May 20, 2020 07:46
@kertal kertal marked this pull request as ready for review May 20, 2020 07:56
@kertal
Copy link
Copy Markdown
Member Author

kertal commented May 20, 2020

@elasticmachine merge upstream

@kertal
Copy link
Copy Markdown
Member Author

kertal commented May 20, 2020

@elasticmachine merge upstream

spalger
spalger approved these changes May 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

test/functional/apps/discover/_indexpattern_without_timefield.ts Outdated
it('should display a timepicker after switching to an index pattern with timefield', async function() {
expect(await PageObjects.timePicker.timePickerExists()).to.be(false);
await PageObjects.discover.selectIndexPattern('with-timefield');
expect(await PageObjects.timePicker.timePickerExists()).to.be(true);
Copy link
Copy Markdown
Contributor

@spalger spalger May 20, 2020

Choose a reason for hiding this comment

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

Unrelated recommendation: I think avoiding boolean assertions makes failures a lot easier to debug. It's often unhelpful to get expected false to be true, but it's really easy to do this:

if (!await PageObjects.timePicker.timePickerExists()) {
  throw new Error('expected timepicker to exist')
}

Copy link
Copy Markdown
Member Author

@kertal kertal May 20, 2020

Choose a reason for hiding this comment

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

this is so true! thx for that! but shouldn't there be at least 1 expect in a test? or isn't it necessary?

Copy link
Copy Markdown
Contributor

@spalger spalger May 20, 2020

Choose a reason for hiding this comment

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

Nope, using expect() is not necessary to write meaningful assertions.

Copy link
Copy Markdown
Member Author

@kertal kertal May 25, 2020

Choose a reason for hiding this comment

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

thx, i've refactored it this way

liza-mae
liza-mae approved these changes May 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@liza-mae liza-mae left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kertal

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 25, 2020

💚 Build Succeeded

History

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

@kertal kertal merged commit 0aac0a3 into elastic:master May 25, 2020
kertal added a commit to kertal/kibana that referenced this pull request May 25, 2020
@kertal
Improve indexpattern without timefield functional test (elastic#67031)
e5c08e3
@kertal kertal mentioned this pull request May 25, 2020
Merged
kertal added a commit to kertal/kibana that referenced this pull request May 25, 2020
@kertal
Improve indexpattern without timefield functional test (elastic#67031)
917c237
@kertal kertal mentioned this pull request May 25, 2020
Merged
kertal added a commit to kertal/kibana that referenced this pull request May 25, 2020
@kertal
Improve indexpattern without timefield functional test (elastic#67031)
5b70e29
@kertal kertal mentioned this pull request May 25, 2020
Merged
kertal added a commit that referenced this pull request May 25, 2020
@kertal
Improve indexpattern without timefield functional test (#67031) (#67266)
14afb84
kertal added a commit that referenced this pull request May 25, 2020
@kertal
Improve indexpattern without timefield functional test (#67031) (#67265)
53f7f13
jloleysens added a commit that referenced this pull request May 26, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/pipel…
f06f9e4 
…ine-editor

* 'master' of github.com:elastic/kibana: (129 commits)
  [Canvas] Force embeddables to refresh when renderable reevaluated (#67133)
  [Canvas] Better handling navigating to/from canvas (#66407)
  [Ingest pipelines] Fix schema validation for simulate and update routes (#67199)
  do not use es from setup (#67277)
  Auto expand replicas for event log (#67286)
  Observability & APM do not use elasticsearch client provided via setup contract  (#67263)
  Fix privileges check when security is not enabled (#67308)
  add IIS home (#66918)
  [ML] Adding additional job service endpoint tests (#66892)
  [Ingest Manager] Update fleet internal doc with latest flags (#67193)
  [Discover] Deangularize the loading spinner (#67165)
  Add `application.navigateToUrl` core API (#67110)
  Improve indexpattern without timefield functional test (#67031)
  KibanaContext in index pattern managment ui (#66985)
  Fix Azure metrics tutorial inside the App Home/ Add data area (#66901)
  add azure logs home (#66910)
  fix: rum agent should work correctly on new platform (#67037)
  [test_utils/Testbed] Move to src/test_utils folder (OSS) (#66898)
  only block registration when appRoute contains the exact basePath (#67125)
  Changed actions API endpoints urls to follow Kibana STYLEGUIDE (#65936)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 27, 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 May 27, 2020
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 28, 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.

4 similar comments
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 29, 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
Copy link
Copy Markdown
Contributor

kibanamachine commented Jun 1, 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
Copy link
Copy Markdown
Contributor

kibanamachine commented Jun 2, 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
Copy link
Copy Markdown
Contributor

kibanamachine commented Jun 3, 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.

kertal added a commit that referenced this pull request Jun 4, 2020
@kertal
[7.7] Improve indexpattern without timefield functional test (#67031) (…
ae69f51 
…#67267)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@spalger spalger spalger approved these changes

@liza-mae liza-mae liza-mae approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kertal kertal

Labels

Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.7.1 v7.8.0 v7.9.0 v8.0.0

Projects

None yet

Milestone

No milestone

5 participants

@kertal @elasticmachine @kibanamachine @spalger @liza-mae