[Reporting] Stabilize CSV export tests #112204

tsullivan · 2021-09-15T02:58:27Z

A CSV Export issue was confirmed to changing the test server's reporting settings to their defaults. Under the default settings (no artificially low maxSizeBytes setting), most CSV Export tests worked on 4675 documents. That amount of data triggered some weak performance in Elasticsearch for the _scroll API, and Reporting tests were failing essentially due to shard failures.

This PR stabilizes the CSV tests. It does so by adjusting the tests use a smaller date range and return back a smaller number of total records. This goal is from here

tsullivan · 2021-09-21T01:06:01Z

x-pack/test/functional/page_objects/reporting_page.ts

-    const fromTime = 'Apr 27, 2019 @ 23:56:51.374';
-    const toTime = 'Aug 23, 2019 @ 16:18:51.821';
+    const fromTime = 'Jun 20, 2019 @ 00:00:00.000';
+    const toTime = 'Jun 25, 2019 @ 00:00:00.000';


The earlier time range created a search that matched 4675 hits, now it is 720

I was only able to reliably reproduce the buggy behaviour with larger CSV exports. I am concerned that if we reduce the number of hits from the search we might hide issues like this in future.

I understand this runs against the changes here that introduced a full snapshot check, but perhaps we could do both tests? One checking a full snapshot (high resolution) and one checking a large export count (with lower resolution snapshot, first 10 and last 10 lines as we had it before).

Let me know what you think!

x-pack/test/functional/page_objects/reporting_page.ts

tsullivan · 2021-09-21T03:21:57Z

x-pack/test/reporting_api_integration/reporting_and_security/generate_csv_discover.ts


-  describe('Generation from Job Params', () => {


All of this removed code used the deprecated export type, and was moved to the new test file: x-pack/test/reporting_api_integration/reporting_and_security/generate_csv_discover_deprecated.ts

tsullivan · 2021-09-21T03:29:37Z

x-pack/test/reporting_api_integration/reporting_and_security/security_roles_privileges.ts

@@ -165,23 +164,21 @@ export default function ({ getService }: FtrProviderContext) {
    describe('Discover: Generate CSV report', () => {
      it('does not allow user that does not have the role-based privilege', async () => {
        const res = await reportingAPI.generateCsv(


Decided to move the username and password parameters to the end of the arguments. This will be useful for more csv_searchsource testing in later PRs.

elasticmachine · 2021-09-21T03:30:23Z

Pinging @elastic/kibana-app-services (Team:AppServices)

elasticmachine · 2021-09-21T03:30:24Z

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

jloleysens · 2021-09-22T08:16:34Z

x-pack/plugins/reporting/server/export_types/csv_searchsource/generate_csv/generate_csv.ts

@@ -345,6 +345,15 @@ export class CsvGenerator {
          break;
        }

+        // TODO check for shard failures, log them and add a warning if found


jloleysens

This is looking great @tsullivan ! I left a few comments that I'd like to get your thoughts on before merging.

jloleysens · 2021-09-22T08:23:10Z

x-pack/test/functional/apps/discover/reporting.ts

      });

      it('generates a report from a new search with data: discover:searchFieldsFromSource', async () => {
        await setFieldsFromSource(true);
        await PageObjects.discover.clickNewSearchButton();
-        await PageObjects.reporting.setTimepickerInDataRange();
+        await PageObjects.reporting.setTimepickerInEcommerceDataRange();


Just after this line it might be useful to add an assertion against the number of hits returned by the search:

expect(await PageObjects.discover.getHitCount()).to.equal('4,675');

That way, if this fails in future, we will know that at least the Discover search is probably working as expected.

jloleysens · 2021-09-22T08:26:26Z

x-pack/test/functional/page_objects/reporting_page.ts

-    const fromTime = 'Apr 27, 2019 @ 23:56:51.374';
-    const toTime = 'Aug 23, 2019 @ 16:18:51.821';
+    const fromTime = 'Jun 20, 2019 @ 00:00:00.000';
+    const toTime = 'Jun 25, 2019 @ 00:00:00.000';


I was only able to reliably reproduce the buggy behaviour with larger CSV exports. I am concerned that if we reduce the number of hits from the search we might hide issues like this in future.

I understand this runs against the changes here that introduced a full snapshot check, but perhaps we could do both tests? One checking a full snapshot (high resolution) and one checking a large export count (with lower resolution snapshot, first 10 and last 10 lines as we had it before).

Let me know what you think!

…ky-test

tsullivan · 2021-09-23T19:12:01Z

I was only able to reliably reproduce the buggy behaviour with larger CSV exports. I am concerned that if we reduce the number of hits from the search we might hide issues like this in future.

I understand this runs against the changes here that introduced a full snapshot check, but perhaps we could do both tests? One checking a full snapshot (high resolution) and one checking a large export count (with lower resolution snapshot, first 10 and last 10 lines as we had it before).

As much as I don't like to use Reporting tests for finding issues with other domains or services, I agree with you since it seems there aren't any other features (or tests) that use _scroll the way that Reporting uses it.

…ky-test

jloleysens · 2021-09-27T08:25:45Z

@elasticmachine merge upstream

jloleysens

Thanks for addressing my feedback @tsullivan !

As much as I don't like to use Reporting tests for finding issues with other domains or services, I agree with you since it seems there aren't any other features (or tests) that use _scroll the way that Reporting uses it.

That concern makes sense to me, perhaps this is an improvement we should follow up on ES side about (i.e., adding tests there for larger scrolls). We could open an issue about it. What do you think?

kibanamachine · 2021-09-27T10:22:49Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #155524 succeeded 428c0d9
💔 Build #155497 failed cc92810
💔 Build #155490 failed c789ce8
💚 Build #155128 succeeded 999f60b
💚 Build #154806 succeeded b8b5477

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

* [Reporting] Stabilize CSV export tests * add debugging logging for results metadata * restore accidentally deleted tests * restore "large export" test * remove redundant availability test * do not filter and re-save * fix getHitCount * fix large export test * skip large export test :( Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

tsullivan · 2021-09-27T20:04:57Z

That concern makes sense to me, perhaps this is an improvement we should follow up on ES side about (i.e., adding tests there for larger scrolls). We could open an issue about it. What do you think?

I'm not sure we need an issue to follow up on things right now, since it looks like the teams are working actively on resolving the issue. Here is another issue where the work is happening: https://github.com/elastic/machine-learning-qa/issues/1125

* [Reporting] Stabilize CSV export tests (#112204) * [Reporting] Stabilize CSV export tests * add debugging logging for results metadata * restore accidentally deleted tests * restore "large export" test * remove redundant availability test * do not filter and re-save * fix getHitCount * fix large export test * skip large export test :( Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> * update snapshots for discover tests * update test snapshots Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

dnhatn · 2021-09-27T22:51:17Z

We have added a test that verifies scroll and search_after requests against a large index.

tsullivan changed the title ~~Reportingg/csv flaky test~~ [Reporting] Address Flaky CSV test Sep 15, 2021

tsullivan force-pushed the reportingg/csv-flaky-test branch from bb66b32 to 2f96ad3 Compare September 15, 2021 17:45

elastic deleted a comment from kibanamachine Sep 15, 2021

tsullivan force-pushed the reportingg/csv-flaky-test branch from a9e102b to 2fd45fd Compare September 16, 2021 02:32

tsullivan requested review from dokmic and jloleysens September 20, 2021 20:24

tsullivan added release_note:fix v7.16.0 v8.0.0 Team:AppServices labels Sep 20, 2021

tsullivan force-pushed the reportingg/csv-flaky-test branch from f748f0a to 4b545a9 Compare September 20, 2021 20:39

elastic deleted a comment from kibanamachine Sep 20, 2021

tsullivan force-pushed the reportingg/csv-flaky-test branch 4 times, most recently from be8350d to c3dd0ba Compare September 21, 2021 01:00

tsullivan commented Sep 21, 2021

View reviewed changes

x-pack/test/functional/page_objects/reporting_page.ts Outdated Show resolved Hide resolved

tsullivan commented Sep 21, 2021

View reviewed changes

tsullivan force-pushed the reportingg/csv-flaky-test branch from 2812074 to afd4336 Compare September 21, 2021 03:27

tsullivan commented Sep 21, 2021

View reviewed changes

tsullivan marked this pull request as ready for review September 21, 2021 03:30

tsullivan requested review from a team as code owners September 21, 2021 03:30

tsullivan marked this pull request as draft September 21, 2021 03:32

tsullivan removed the release_note:fix label Sep 21, 2021

add debugging logging for results metadata

b8b5477

tsullivan marked this pull request as ready for review September 21, 2021 23:53

elastic deleted a comment from kibanamachine Sep 21, 2021

jloleysens reviewed Sep 22, 2021

View reviewed changes

tsullivan added 2 commits September 22, 2021 13:40

Merge remote-tracking branch 'elastic/master' into reportingg/csv-fla…

dd5f527

…ky-test

restore accidentally deleted tests

999f60b

tsullivan added 7 commits September 23, 2021 12:15

Merge remote-tracking branch 'elastic/master' into reportingg/csv-fla…

5a9d2c5

…ky-test

restore "large export" test

7000b25

remove redundant availability test

b4350a8

do not filter and re-save

eb32dbf

fix getHitCount

c789ce8

fix large export test

cc92810

skip large export test :(

428c0d9

Merge branch 'master' into reportingg/csv-flaky-test

d7bae83

jloleysens approved these changes Sep 27, 2021

View reviewed changes

tsullivan merged commit cb37ae8 into elastic:master Sep 27, 2021

tsullivan mentioned this pull request Sep 27, 2021

[7.x] [Reporting] Stabilize CSV export tests (#112204) #113158

Merged

tsullivan deleted the reportingg/csv-flaky-test branch September 27, 2021 20:03

tsullivan mentioned this pull request Sep 1, 2022

[Reporting] Test needed: "Generate CSV" export from Discover, verify the download against a snapshot #133836

Closed

sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Stabilize CSV export tests #112204

[Reporting] Stabilize CSV export tests #112204

tsullivan commented Sep 15, 2021 •

edited

Loading

tsullivan Sep 21, 2021

jloleysens Sep 22, 2021

tsullivan Sep 21, 2021 •

edited

Loading

tsullivan Sep 21, 2021

elasticmachine commented Sep 21, 2021

elasticmachine commented Sep 21, 2021

jloleysens Sep 22, 2021

jloleysens left a comment

jloleysens Sep 22, 2021

jloleysens Sep 22, 2021

tsullivan commented Sep 23, 2021

jloleysens commented Sep 27, 2021

jloleysens left a comment

kibanamachine commented Sep 27, 2021

tsullivan commented Sep 27, 2021

dnhatn commented Sep 27, 2021

[Reporting] Stabilize CSV export tests #112204

[Reporting] Stabilize CSV export tests #112204

Conversation

tsullivan commented Sep 15, 2021 • edited Loading

tsullivan Sep 21, 2021

Choose a reason for hiding this comment

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

tsullivan Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

tsullivan Sep 21, 2021

Choose a reason for hiding this comment

elasticmachine commented Sep 21, 2021

elasticmachine commented Sep 21, 2021

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

jloleysens Sep 22, 2021

Choose a reason for hiding this comment

tsullivan commented Sep 23, 2021

jloleysens commented Sep 27, 2021

jloleysens left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 27, 2021

💚 Build Succeeded

Metrics [docs]

History

tsullivan commented Sep 27, 2021

dnhatn commented Sep 27, 2021

tsullivan commented Sep 15, 2021 •

edited

Loading

tsullivan Sep 21, 2021 •

edited

Loading