-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[6.3] Improve visualization tests #20783
Conversation
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
This is a manual back port of #20782, plus disable of tile map test. |
Is this a backport, or do you intend on forward porting this after merging? |
@tylersmalley this is the master/6.x version was waiting to merge this: #20782 |
@@ -88,6 +88,7 @@ export default function ({ getService, getPageObjects }) { | |||
*/ | |||
it('should configure Terms aggregation on play_name', async function () { | |||
await PageObjects.visualize.clickBucket('X-Axis'); | |||
await PageObjects.common.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a 500ms sleep in the clickBucket method.
https://github.com/liza-mae/kibana/blob/3e3f76d09b6eb916249c1c833b2c7d3613dfa0db/test/functional/page_objects/visualize_page.js#L397
Should we just increase that sleep instead of adding this one to the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately what I found is sometimes specific tests take longer and that was the case here, these were run in a loop and adding the sleep there helped, but I see some changes are pending for clickBucket, so let me take those in.
@@ -30,7 +30,8 @@ export default function ({ getService, getPageObjects }) { | |||
log.debug(`Interval = ${providedInterval}`); | |||
await PageObjects.visualize.setNumericInterval(providedInterval); | |||
await PageObjects.visualize.clickGo(); | |||
await PageObjects.header.waitUntilLoadingHasFinished(); | |||
await PageObjects.common.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the sleep to the clickGo() method?
Or add a call to waitForVisualization()
in the clickGo method?
https://github.com/liza-mae/kibana/blob/3e3f76d09b6eb916249c1c833b2c7d3613dfa0db/test/functional/page_objects/visualize_page.js#L824
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I tried both those for this function and the only that helped was a sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on why the vega test is skipped. I didn't see it mentioned in the PR description. Is it passing on Jenkins but failing on Cloud? Or flaky on Jenkins?
There's a few sleeps being added to tests which I think might be better in the page object methods so that all calls to the method get the sleep.
It looks like we could add a call to waitForVisualization() in a few places that could reduce the need for sleeps. But I haven't tried that myself so I'll leave that up to you if you want to look into it.
@@ -18,7 +18,7 @@ export default function ({ getService, getPageObjects }) { | |||
expect(spyToggleExists).to.be(false); | |||
}); | |||
|
|||
it('should have some initial vega spec text', async function () { | |||
it.skip('should have some initial vega spec text', async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue for this new skip on this vega test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check your issue #20777 for the issue number on vega, it was already skipped and not part of my PR I just thought I would try it but it failed so I reverted it back to skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Skipped tests issue only looked at master. That vega test isn't currently skipped on 6.3;
https://github.com/elastic/kibana/blob/6.3/test/functional/apps/visualize/_vega_chart.js#L21
It could be a flaky test and maybe should be skipped on 6.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right on 6.3 it is not skipped, it is flaky since it passed for me initially but then failed on some later run, so I disabled it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, if it's not something you can easily fix and need to skip it, please also create an issue with the failure details, screenshot, etc. and reference it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a look to see if I can fix.
💚 Build Succeeded |
jenkins test this |
💚 Build Succeeded |
jenkins test this |
💚 Build Succeeded |
jenkins test this |
💚 Build Succeeded |
This PR is an attempt to improve the visualization tests. This PR should cover issues:
#20388 flaky histogram tests
#20387 flaky tag cloud tests
#20153 skip tile map test
It may improve issues with input control.
Also re-enabling remaining skipped tests in input control, tsvb and linked saved searches.