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

[Discover] Unskip functional tests for field visualize buttons #62614

Merged
merged 27 commits into from
Apr 22, 2020
Merged

[Discover] Unskip functional tests for field visualize buttons #62614

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ca8965b
Unskip test
kertal Apr 2, 2020
12fdaba
Add visualize button test in OSS
kertal Apr 3, 2020
3e9b076
Remove other discover tests
kertal Apr 4, 2020
e55fdde
Implement retry for flaky tests
kertal Apr 5, 2020
9e3e00a
Remove before code that failed
kertal Apr 6, 2020
afc9ff3
Improve retry code
kertal Apr 6, 2020
f1ee00f
Apply fix to discover security and spaces
kertal Apr 6, 2020
021c32d
Merge upstream/master, fix conflicts
kertal Apr 6, 2020
ff5895e
Undo _discover.js changes
kertal Apr 6, 2020
0088147
Add missing retry service to discover_security.ts
kertal Apr 6, 2020
d318d32
Fix security bugs
kertal Apr 6, 2020
f95faa7
Merge branch 'master' into kertal-pr-2020-04-02-discover-unskip-field…
elasticmachine Apr 10, 2020
e73cd0b
Adapt tests to use PageObjects.discover.getHitCount()
kertal Apr 16, 2020
d9737d3
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-04…
kertal Apr 16, 2020
d11190d
Merge branch 'kertal-pr-2020-04-02-discover-unskip-field-visualize-fu…
kertal Apr 16, 2020
8cff5ef
fix tests that expect the visualize button not to exist
kertal Apr 16, 2020
1a368b3
Add debugging
kertal Apr 17, 2020
49974e4
Increase waitForCompletionTimeout to 5s
kertal Apr 17, 2020
a50a210
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-04…
kertal Apr 18, 2020
cc76e78
Undo test adaptions
kertal Apr 18, 2020
1986411
Modify async search code
kertal Apr 22, 2020
6688d68
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-04…
kertal Apr 22, 2020
51f52bc
undo wait for completion timeout setting too 5s
kertal Apr 22, 2020
ba1dbbf
resolves #64132
Apr 22, 2020
c6688fb
simplify condition
Apr 22, 2020
934db8e
Merge async fix
kertal Apr 22, 2020
63aa5df
Merge upstream/master, fix conflicts
kertal Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions x-pack/test/functional/apps/discover/feature_controls/discover_security.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { FtrProviderContext } from '../../../ftr_provider_context';

export default function({ getPageObjects, getService }: FtrProviderContext) {
const esArchiver = getService('esArchiver');
const retry = getService('retry');
const security = getService('security');
const globalNav = getService('globalNav');
const config = getService('config');
Expand All @@ -28,8 +29,7 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
await PageObjects.timePicker.setDefaultAbsoluteRange();
}

// FLAKY: https://github.com/elastic/kibana/issues/60535
describe.skip('security', () => {
describe('security', () => {
before(async () => {
await esArchiver.load('discover/feature_controls/security');
await esArchiver.loadIfNeeded('logstash_functional');
Expand Down Expand Up @@ -185,9 +185,14 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {

it(`doesn't show visualize button`, async () => {
await PageObjects.common.navigateToApp('discover');
await setDiscoverTimeRange();
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
await retry.try(async () => {
await setDiscoverTimeRange();
const hasNoResults = await PageObjects.discover.hasNoResults();
Copy link

Choose a reason for hiding this comment

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

I would generally try to avoid checking for something to not exist since it takes a timeout of 10 seconds or so. Compared to checking for something that should exist like the hit count. Don't change anything yet. I'm going to run these tests locally and see if I have a suggestion for a change.

Copy link

Choose a reason for hiding this comment

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

The current code as it retries a couple of times to make sure it's not on the "no results" page (it's not the default timeout, but a 2500ms timeout) takes about 3 seconds;

[11:39:34.333150000] │ debg TestSubjects.exists(discoverNoResults)
[11:39:34.393075600] │ debg Find.existsByDisplayedByCssSelector('[data-test-subj="discoverNoResults"]') with timeout=2500
[11:39:34.704008100] │ debg --- retry.tryForTime error: [data-test-subj="discoverNoResults"] is not displayed
[11:39:35.236323000] │ debg --- retry.tryForTime failed again with the same message...
[11:39:35.761950200] │ debg --- retry.tryForTime failed again with the same message...
[11:39:36.293043400] │ debg --- retry.tryForTime failed again with the same message...
[11:39:36.829574800] │ debg --- retry.tryForTime failed again with the same message...
[11:39:37.354057600] │ debg TestSubjects.click(field-bytes)

vs getting the hitCount and verifying it's > 0 takes about .2 seconds

[11:54:10.954650700] │ debg TestSubjects.getVisibleText(discoverQueryHits)
[11:54:11.002496700] │ debg TestSubjects.find(discoverQueryHits)
[11:54:11.052031400] │ debg Find.findByCssSelector('[data-test-subj="discoverQueryHits"]') with timeout=10000
[11:54:11.113423700] │ debg TestSubjects.click(field-bytes)

@@ -187,8 +187,9 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
         await PageObjects.common.navigateToApp('discover');
         await retry.try(async () => {
           await setDiscoverTimeRange();
-          const hasNoResults = await PageObjects.discover.hasNoResults();
-          expect(hasNoResults).to.be(false);
+          const hitCount = await PageObjects.discover.getHitCount();
+          // eslint-disable-next-line radix
+          expect(parseInt(hitCount)).to.be.greaterThan(0);

           await PageObjects.discover.clickFieldListItem('bytes');
           await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
@@ -281,8 +282,10 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
         await PageObjects.common.navigateToApp('discover');
         await retry.try(async () => {
           await setDiscoverTimeRange();
-          const hasNoResults = await PageObjects.discover.hasNoResults();
-          expect(hasNoResults).to.be(false);
+          const hitCount = await PageObjects.discover.getHitCount();
+          // eslint-disable-next-line radix
+          expect(parseInt(hitCount)).to.be.greaterThan(0);
+
           await PageObjects.discover.clickFieldListItem('bytes');
           await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
         });
@@ -362,8 +365,9 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
         await PageObjects.common.navigateToApp('discover');
         await retry.try(async () => {
           await setDiscoverTimeRange();
-          const hasNoResults = await PageObjects.discover.hasNoResults();
-          expect(hasNoResults).to.be(false);
+          const hitCount = await PageObjects.discover.getHitCount();
+          // eslint-disable-next-line radix
+          expect(parseInt(hitCount)).to.be.greaterThan(0);

Copy link

Choose a reason for hiding this comment

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

Sorry but I just realized another potential issue with this change.

In other Discover tests we've used a retry only around getting the hit count and comparing it to the expected value. We didn't include setting the time range in the retry because each time you set the timepicker it's going to reload the page, and it's the page loading we're waiting for with the retry.

From the failing test issue you said
"the screenshot of the failed test is telling me, no data available, expand your time range. that's odd"

Did the screenshot show the expected start and end dates?

Copy link
Member Author

Choose a reason for hiding this comment

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

thx @LeeDr , back today, I'll soon provide feeback

Copy link
Member Author

Choose a reason for hiding this comment

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

About the screenshot, yes it's showing the defined time range, but no data:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

I've run a similar test suite in OSS for debugging the issue, it's wasn't flaky there:
https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/339/

Copy link
Member Author

Choose a reason for hiding this comment

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

dear @LeeDr, wonder how to proceed here?

Maybe switch to const hitCount = await PageObjects.discover.getHitCount(); , since this fixes the test, and open another issue because of the flaky data fetching to investigate?

Thx!

Copy link

Choose a reason for hiding this comment

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

I find it pretty concerning that the screenshot shows the correct dates in the timepicker and no results?!?! It could still just be a timing issue that the results just haven't come back in the response yet, but the pink loading bar isn't there either so that doesn't feel right.

I'm looking at the flaky-test-suite-runner output now....

FYI, here's an example of a test where we only put the getHitCount() in the retry because it's waiting for the response from Elasticsearch and for the page to load that data; https://github.com/elastic/kibana/blob/master/test/functional/apps/discover/_discover.js#L74

Copy link
Member Author

Choose a reason for hiding this comment

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

@LeeDr I've adapted the code, removing setDiscoverTimerRange() of try.retry, now the flaky suite is flaky (1 of 44)
https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/367/

image

can I search the logs on server? because in Jenkins it's hard to search the logs, it says, no test failed

Copy link

Choose a reason for hiding this comment

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

/cc @lukasolson (any other thoughts on this?)

I had a couple of thoughts on debugging this while running the test locally.

We could turn on Elasticsearch slowlogs on both the logstash-* and .async-search indices. I don't see that we've done that in any existing tests yet. It's a per-index setting. Seems like it would have to be done after esArchiver.loadIfNeeded('logstash_functional');. But the slowlog only shows the query, not the response. So this might not help in debugging the issue.

Another thing you could try, is if we fail to find hit count, or if we do find the "no results" page, is to try to open the inspector and capture the request and response. It could show that either the query sent was wrong, or the query was right and Elasticsearch didn't return the correct response, or the correct response was returned and Discover didn't display it.

Or temporarily add debug logging to output the query and response to the Kibana log.

expect(hasNoResults).to.be(false);

await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
});
});

it(`Permalinks doesn't show create short-url button`, async () => {
Expand Down Expand Up @@ -274,9 +279,13 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {

it(`doesn't show visualize button`, async () => {
await PageObjects.common.navigateToApp('discover');
await setDiscoverTimeRange();
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
await retry.try(async () => {
await setDiscoverTimeRange();
const hasNoResults = await PageObjects.discover.hasNoResults();
expect(hasNoResults).to.be(false);
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
});
});

it('Permalinks shows create short-url button', async () => {
Expand Down Expand Up @@ -351,9 +360,14 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {

it(`shows the visualize button`, async () => {
await PageObjects.common.navigateToApp('discover');
await setDiscoverTimeRange();
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectFieldListItemVisualize('bytes');
await retry.try(async () => {
await setDiscoverTimeRange();
const hasNoResults = await PageObjects.discover.hasNoResults();
expect(hasNoResults).to.be(false);

await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectFieldListItemVisualize('bytes');
});
});
});

Expand Down
25 changes: 17 additions & 8 deletions x-pack/test/functional/apps/discover/feature_controls/discover_spaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
const esArchiver = getService('esArchiver');
const config = getService('config');
const spacesService = getService('spaces');
const retry = getService('retry');
const PageObjects = getPageObjects([
'common',
'discover',
Expand All @@ -24,8 +25,7 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
await PageObjects.timePicker.setDefaultAbsoluteRange();
}

// FLAKY: https://github.com/elastic/kibana/issues/60559
describe.skip('spaces', () => {
describe('spaces', () => {
before(async () => {
await esArchiver.loadIfNeeded('logstash_functional');
});
Expand Down Expand Up @@ -68,9 +68,14 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
await PageObjects.common.navigateToApp('discover', {
basePath: '/s/custom_space',
});
await setDiscoverTimeRange();
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectFieldListItemVisualize('bytes');
await retry.try(async () => {
await setDiscoverTimeRange();
const hasNoResults = await PageObjects.discover.hasNoResults();
expect(hasNoResults).to.be(false);

await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectFieldListItemVisualize('bytes');
});
});
});

Expand Down Expand Up @@ -134,9 +139,13 @@ export default function({ getPageObjects, getService }: FtrProviderContext) {
await PageObjects.common.navigateToApp('discover', {
basePath: '/s/custom_space',
});
await setDiscoverTimeRange();
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
await retry.try(async () => {
await setDiscoverTimeRange();
const hasNoResults = await PageObjects.discover.hasNoResults();
expect(hasNoResults).to.be(false);
await PageObjects.discover.clickFieldListItem('bytes');
await PageObjects.discover.expectMissingFieldListItemVisualize('bytes');
});
});
});

Expand Down