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.

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

Async Discover search test #64388

Merged
merged 26 commits into from
Jun 29, 2020
Merged

Async Discover search test #64388

merged 26 commits into from
Jun 29, 2020

Conversation

LeeDr
Copy link

@LeeDr LeeDr commented Apr 24, 2020

Summary

This PR is for new functional UI tests in x-pack for Discover with async search.

There are currently 2 tests;
query should show failed shards pop up
query return results with valid scripted field (fails on 7.7.0 BC8)

The trick to these tests are in the scripted fields. The test originally started with "empty_kibana" .kibana index and created the index pattern and scripted fields through the UI. But the last commit has that already set in the kibana_scripted_fields_on_logstash esArchive to make the test much faster.
With a couple of scripted fields that cause Elasticsearch to have to do some work, the initial response from Elasticsearch in that case wasn't being handled properly and would cause the "no results" message to show. With the fixes which are already merged the query returns the expected results.

The first test causes the failed shards popup. A recent fix in Elasticsearch fixed this.

NOTE: One thing unusually about the test file in it's current version is that it has all the steps commented-out which were used to create the scripted fields which are now in the esArchive. I know we don't usually like to leave a bunch of commented-out code in our files but this might be a practice to consider. If someone needed to change what was in the esArchive it would be pretty painful without those steps.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@LeeDr LeeDr requested a review from kertal April 29, 2020 15:52
@kertal
Copy link
Member

kertal commented Apr 29, 2020

ACK & THX @LeeDr, will review it tomorrow

@LeeDr
Copy link
Author

LeeDr commented Apr 30, 2020

Results of the 2 new tests;

x-pack/test/functional/apps/discover/async_scripted_fields·js 1 min 12 sec 0   0   2 +2 2 +2

@LeeDr
Copy link
Author

LeeDr commented Apr 30, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented May 5, 2020

started a flaky test runner for it, looks stable: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/424/

@LeeDr LeeDr added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure test_xpack_functional v7.8.0 v8.0.0 labels May 7, 2020
@elasticmachine
Copy link
Contributor

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

@LeeDr
Copy link
Author

LeeDr commented May 8, 2020

I ran the new test 42 times on the flaky test runner and all passed.

@LeeDr
Copy link
Author

LeeDr commented May 8, 2020

@elasticmachine merge upstream

@LeeDr
Copy link
Author

LeeDr commented May 13, 2020

@elasticmachine merge upstream

@LeeDr LeeDr changed the title Async search test Async Discover search test May 13, 2020
@LeeDr
Copy link
Author

LeeDr commented May 15, 2020

Last Jenkins job test failure indicates some problem loading data;

[00:09:11]             └-> "before all" hook
[00:09:11]               │ info [kibana_scripted_fields_on_logstash] Loading "mappings.json"
[00:09:11]               │ info [kibana_scripted_fields_on_logstash] Loading "data.json.gz"
[00:09:11]               │ info [o.e.c.m.MetadataDeleteIndexService] [kibana-ci-immutable-ubuntu-16-tests-xl-1589374883702750076] [.kibana_2/JGNjvdJ6RlmhSgo3Rld8GA] deleting index
[00:09:11]               │ info [o.e.c.m.MetadataDeleteIndexService] [kibana-ci-immutable-ubuntu-16-tests-xl-1589374883702750076] [.kibana_1/EBvRVAWZQ_idG5p26KCaOw] deleting index
[00:09:11]               │ info [kibana_scripted_fields_on_logstash] Deleted existing index [".kibana_2",".kibana_1"]
[00:09:11]               │ info Taking screenshot "/dev/shm/workspace/kibana/x-pack/test/functional/screenshots/failure/discover async search with scripted fields _before all_ hook.png"
[00:09:11]               │ info Current URL is: http://localhost:6191/s/another-space/app/discover#/c760fbb0-9522-11ea-963a-eba904777d7f?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_a=(columns:!(_source),filters:!(),index:%27logstash-*%27,interval:auto,query:(language:kuery,query:%27%27),sort:!())
[00:09:12]               │ info Saving page source to: /dev/shm/workspace/kibana/x-pack/test/functional/failure_debug/html/discover async search with scripted fields _before all_ hook.html
[00:09:12]               └- ✖ fail: "discover async search with scripted fields "before all" hook for "query should show failed shards pop up""
Stacktrace
{ Error: [illegal_argument_exception] unknown setting [index.prefer_v2_templates] please check that any required plugins are installed, or check the breaking changes documentation for removed settings
    at respond (/dev/shm/workspace/kibana/node_modules/elasticsearch/src/lib/transport.js:349:15)
    at checkRespForFailure (/dev/shm/workspace/kibana/node_modules/elasticsearch/src/lib/transport.js:306:7)
    at HttpConnector.<anonymous> (/dev/shm/workspace/kibana/node_modules/elasticsearch/src/lib/connectors/http.js:173:7)
    at IncomingMessage.wrapper (/dev/shm/workspace/kibana/node_modules/elasticsearch/node_modules/lodash/lodash.js:4929:19)
    at endReadableNT (_stream_readable.js:1145:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
  status: 400,
  displayName: 'BadRequest',
  message:
   '[illegal_argument_exception] unknown setting [index.prefer_v2_templates] please check that any required plugins are installed, or check the breaking changes documentation for removed settings',
  path: '/.kibana_1',

@LeeDr
Copy link
Author

LeeDr commented May 15, 2020

@elasticmachine merge upstream

@LeeDr
Copy link
Author

LeeDr commented Jun 24, 2020

@elasticmachine merge upstream

@LeeDr LeeDr added v7.9.0 and removed v7.8.1 labels Jun 24, 2020
@LeeDr LeeDr marked this pull request as ready for review June 24, 2020 18:13
@LeeDr
Copy link
Author

LeeDr commented Jun 24, 2020

@kertal remember this PR? :-) It fell out of date but I've got it up and passing again if you have a chance to review it.

@kertal
Copy link
Member

kertal commented Jun 25, 2020

@LeeDr thx, sure, how could I forget :), will review it

Comment on lines +27 to +31
// changing the timepicker default here saves us from having to set it in Discover (~8s)
await kibanaServer.uiSettings.update({
'timepicker:timeDefaults':
'{ "from": "Sep 18, 2015 @ 19:37:13.000", "to": "Sep 23, 2015 @ 02:30:09.000"}',
});
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is nice, imagine how much time would be saved if we regularly do it that way. which reminds me, I recently created a helper for that, in the TimePickerProvider, but it's using a different start, end time

async setDefaultAbsoluteRangeViaUiSettings() {

Comment on lines +35 to +36
await kibanaServer.uiSettings.replace({});
await kibanaServer.uiSettings.update({});
Copy link
Member

Choose a reason for hiding this comment

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

this is just a question, there is no way to replace a single uiSetting ? just asking because of

async resetDefaultAbsoluteRangeViaUiSettings() {

Comment on lines 42 to 59
/* If you had to modify the scripted fields, you could un-comment all this, run it, use es_archiver to update 'kibana_scripted_fields_on_logstash'
*/

// await PageObjects.settings.navigateTo();
// await PageObjects.settings.clickKibanaIndexPatterns();
// await PageObjects.settings.createIndexPattern('logsta');
// await PageObjects.settings.clickScriptedFieldsTab();
// await log.debug('add scripted field');
// await PageObjects.settings.addScriptedField(
// 'sharedFail',
// 'painless',
// 'string',
// null,
// '1',
// // Scripted field below with multiple string checking actually should cause share failure message
// // bcause it's not checking if all the fields it uses exist in each doc (and they don't)
// "if (doc['response.raw'].value == '200') { return 'good ' + doc['url.raw'].value } else { return 'bad ' + doc['machine.os.raw'].value } "
// );
Copy link
Member

Choose a reason for hiding this comment

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

yes, makes sense to keeps this here

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.

Code LGTM, minor remarks and questions added, maybe a final flaky test suite linked in the description of this PR. Crazy how much time it took the reproduce this problem once, and it's now done in a few lines of code 👍

Comment on lines 73 to 110
// the commented-out steps below were used to create the scripted fields in the logstash-* index pattern
// which are now saved in the esArchive.

// await PageObjects.settings.navigateTo();
// await PageObjects.settings.clickKibanaIndexPatterns();
// await PageObjects.settings.clickIndexPatternLogstash();
// const startingCount = parseInt(await PageObjects.settings.getScriptedFieldsTabCount());
// await PageObjects.settings.clickScriptedFieldsTab();
// await log.debug('add scripted field');
// await PageObjects.settings.addScriptedField(
// 'goodScript',
// 'painless',
// 'string',
// null,
// '1',
// // Scripted field below with should work
// "if (doc['response.raw'].value == '200') { if (doc['url.raw'].size() > 0) { return 'good ' + doc['url.raw'].value } else { return 'good' } } else { if (doc['machine.os.raw'].size() > 0) { return 'bad ' + doc['machine.os.raw'].value } else { return 'bad' } }"
// );
// await retry.try(async function() {
// expect(parseInt(await PageObjects.settings.getScriptedFieldsTabCount())).to.be(
// startingCount + 1
// );
// });

// await PageObjects.settings.addScriptedField(
// 'goodScript2',
// 'painless',
// 'string',
// null,
// '1',
// // Scripted field below which should work
// "if (doc['url.raw'].size() > 0) { String tempString = \"\"; for ( int i = (doc['url.raw'].value.length() - 1); i >= 0 ; i--) { tempString = tempString + (doc['url.raw'].value).charAt(i); } return tempString; } else { return \"emptyUrl\"; }"
// );
// await retry.try(async function() {
// expect(parseInt(await PageObjects.settings.getScriptedFieldsTabCount())).to.be(
// startingCount + 2
// );
// });
Copy link
Member

Choose a reason for hiding this comment

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

just wonder if it would make sense, extracting these code into helper functions, uncommenting the usage of those?

@LeeDr
Copy link
Author

LeeDr commented Jun 29, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@LeeDr LeeDr merged commit 752fa6e into elastic:master Jun 29, 2020
@LeeDr LeeDr deleted the asyncSearchTest branch June 29, 2020 20:40
LeeDr pushed a commit to LeeDr/kibana that referenced this pull request Jun 29, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 30, 2020
…ata-streams

* 'master' of github.com:elastic/kibana: (50 commits)
  [Logs UI] [Alerting] "Group by" functionality (elastic#68250)
  [Discover] Deangularize Skip to bottom button (elastic#69811)
  Implement recursive plugin discovery (elastic#68811)
  Use ts-expect-error in platform code (elastic#69883)
  [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend
  [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208)
  [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603)
  [ILM] Fix bug when clearing priority field (elastic#70154)
  [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139)
  [IngestManager] Allow to filter agent by packages (elastic#69731)
  [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199)
  [Metrics UI] UX improvements for saved views (elastic#69910)
  [APM] docs: unique transaction troubleshooting (elastic#69831)
  Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007)
  [Maps] choropleth layer wizard (elastic#69699)
  Make custom errors by extending Error (elastic#69966)
  [Ingest Manager] Support updated package output structure (elastic#69864)
  Resolver test coverage (elastic#70246)
  Async Discover search test (elastic#64388)
  [ui-shared-deps] include styled-components (elastic#69322)
  ...

# Conflicts:
#	x-pack/plugins/snapshot_restore/server/types.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 30, 2020
…bana into alerting/consumer-based-rbac

* 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (49 commits)
  [Discover] Deangularize Skip to bottom button (elastic#69811)
  Implement recursive plugin discovery (elastic#68811)
  Use ts-expect-error in platform code (elastic#69883)
  [SIEM][Detection Engine][Lists] Moves getQueryFilter to common folder for use by both front and backend
  [Ingest Manager][SECURITY SOLUTION] adjust config reassign link and add roundtrip to Reassignment flow (elastic#70208)
  [Security][Lists] Add API functions and react hooks for value list APIs (elastic#69603)
  [ILM] Fix bug when clearing priority field (elastic#70154)
  [Platform][Security] Updates cluster_manager ignorePaths to include security scripts (elastic#70139)
  [IngestManager] Allow to filter agent by packages (elastic#69731)
  [code coverage] exclude folders: test_helpers, tests_bundle (elastic#70199)
  [Metrics UI] UX improvements for saved views (elastic#69910)
  [APM] docs: unique transaction troubleshooting (elastic#69831)
  Cross cluster search functional test with minimun privileges assigned to the test_user (elastic#70007)
  [Maps] choropleth layer wizard (elastic#69699)
  Make custom errors by extending Error (elastic#69966)
  [Ingest Manager] Support updated package output structure (elastic#69864)
  Resolver test coverage (elastic#70246)
  Async Discover search test (elastic#64388)
  [ui-shared-deps] include styled-components (elastic#69322)
  SECURITY-ENDPOINT: add host properties (elastic#70238)
  ...
LeeDr pushed a commit that referenced this pull request Jun 30, 2020
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure test_xpack_functional v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants