ROR_INTR_001 #3

lodyczekoladowe · 2024-08-14T21:34:41Z

Added test for generating rapports form data.

Task description

elk-ror/conf/kbn/kibana.yml

elk-ror/conf/es/readonlyrest.yml

e2e-tests/cypress.config.ts

e2e-tests/cypress/e2e/Report-generation.cy.ts

e2e-tests/cypress/support/page-objects/Login.ts

e2e-tests/cypress/support/page-objects/management/DataViews.ts

e2e-tests/cypress/support/page-objects/management/Reports.ts

coutoPL · 2024-08-15T09:17:37Z

e2e-tests/cypress/e2e/Report-generation.cy.ts

@@ -0,0 +1,109 @@
+// report-generation.spec.ts


when I run the single test in cypress UI I got: "Invalid Version: NOT_SET_YET". I recall you had this problem. Could you please tell us how to solve it?

You need to use $ cd e2e-tests; yarn cypress open --env kibanaVersion=[kbnVersion], for example $ cd e2e-tests; yarn cypress open --env kibanaVersion=7.17.22.

README.md file

Cypress tests in interactive GUI

$ cd e2e-tests; yarn cypress open

I run it locally (8.14.3) and I got sth like this:

@Dzuming could you please check on your side?

I've checked for KBN 7.17.22 and it works. So it seems the problem is related to KBN 8.x

Yes, I can confirm. Seems that the test code should be adjusted to the Kibana 8.x -> We should change 8.4.3 to the 8.5.0 version since it's the newest supported version.

@Dzuming you meant 8.14.3 to 8.15.0? I will change it on the master branch
@lodyczekoladowe so, please take a look at the problem with this test in case of 8.x

you meant 8.14.3 to 8.15.0?

Yes, exactly

Fixed in Fixes for new version of es

Dzuming

I left some comments but looks promising

e2e-tests/cypress/e2e/Report-generation.cy.ts

e2e-tests/cypress/support/page-objects/management/Reports.ts

e2e-tests/cypress/support/commands.ts

e2e-tests/cypress/e2e/Report-generation.cy.ts

Dzuming · 2024-08-16T06:43:45Z

e2e-tests/cypress/support/page-objects/management/Reports.ts

+    cy.get('tr[data-test-subj="reportJobRow"]').should('not.exist');
+  }
+
+  static deleteAllReports(reportTitle: string = 'Untitled discover search') {


This logic seems super complicated and fragile based on the Kibana version. Maybe we can just use the curl command to remove the reporting index?

It seems to me that it is not possible to delete these reports using the curl command.

@lodyczekoladowe What about this comment?

It seems to me that it is not possible to delete these reports using the curl command.

It should be possible (but also complicated). My proposal, was, to delete the reporting index by the Kibana Api
https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-delete-index.html, thanks to it, all existing reports will be cleared

That command delate index but don't delate reports. There is no curl command that deletes reports.

When you remove an ..reporting index that stores reports, you also remove reports :)

Dzuming · 2024-08-30T05:33:19Z

@lodyczekoladowe I can see that you resolved all comments, the good practice is to let resolve a comment from the person who started a conversation. Thanks to it it's easier to track if changes were applied or discuss further. If you resolve an issue, you can leave a message in a thread (for example "resolved"), and I, as a reviewer, will know that I can re-review this specific change. Also, as I see, some changes were not applied, without any comment. Could you leave some comments?

lodyczekoladowe · 2024-08-30T10:09:55Z

@lodyczekoladowe I can see that you resolved all comments, the good practice is to let resolve a comment from the person who started a conversation. Thanks to it it's easier to track if changes were applied or discuss further. If you resolve an issue, you can leave a message in a thread (for example "resolved"), and I, as a reviewer, will know that I can re-review this specific change. Also, as I see, some changes were not applied, without any comment. Could you leave some comments?

Okay, I'll leave comments.

lodyczekoladowe · 2024-09-02T10:35:34Z

@lodyczekoladowe I can see that you resolved all comments, the good practice is to let resolve a comment from the person who started a conversation. Thanks to it it's easier to track if changes were applied or discuss further. If you resolve an issue, you can leave a message in a thread (for example "resolved"), and I, as a reviewer, will know that I can re-review this specific change. Also, as I see, some changes were not applied, without any comment. Could you leave some comments?

Okay, I'll leave comments.

OK, it's done

coutoPL · 2024-09-02T16:25:05Z

e2e-tests/cypress/support/page-objects/Discover.ts

+    }
+  }
+
+  static generateCsvReport(timeout: number = 50000) {


currently, we know nothing about the timeout unit. When you use a type that doesn't support it, it's a good practise to add a hint in the param name e.g. timeoutSeconds

coutoPL · 2024-09-02T16:54:11Z

e2e-tests/cypress/support/page-objects/Discover.ts

+    } else {
+      triger = 'CSV created for';
+    }
+    // Attempt to find the "Created report for" message within the timeout


IMO it should work a little bit differently. In this test, we want to check if many reports running at the same time are going to work as expected. In the past, we have a problem with it. When we run reports one by one (waiting for one finish before starting another) everything was OK. But when we run many reports (and the reports generation was scheduled by Kibana to be executed by a different node than the node where the report was scheduled on), there was a problem. This is what we want to test in this task.

So, we want to run many reports one after another. Then we should go to the view where the reports are shown (management sth view) and wait there for the generation finish. When at least one fails, the test should fail.

It’s not possible with such a small amount of data—the tests finish too quickly. Since a toast notification pops up every time a report is added to the queue, we have to either remove it or wait for it to disappear, because soon the number of these notifications will block our access to generating new reports.

as we talked:

let's generate huge number of docs

let's cut off the services resources

Fixed in Self-generate invoices data, checking report generation process

Dzuming · 2024-09-02T18:03:46Z

e2e-tests/cypress/e2e/Report-generation.cy.ts

@@ -0,0 +1,109 @@
+// report-generation.spec.ts


Yes, I can confirm. Seems that the test code should be adjusted to the Kibana 8.x -> We should change 8.4.3 to the 8.5.0 version since it's the newest supported version.

Dzuming · 2024-09-02T18:04:28Z

e2e-tests/cypress/support/page-objects/management/Reports.ts

+    cy.get('tr[data-test-subj="reportJobRow"]').should('not.exist');
+  }
+
+  static deleteAllReports(reportTitle: string = 'Untitled discover search') {


@lodyczekoladowe What about this comment?

e2e-tests/cypress/support/page-objects/management/Reports.ts

e2e-tests/cypress/e2e/Report-generation.cy.ts

# Conflicts: # e2e-tests/cypress.config.ts # e2e-tests/cypress/support/commands.ts # e2e-tests/cypress/support/e2e.ts # e2e-tests/cypress/support/page-objects/Discover.ts # e2e-tests/package.json # e2e-tests/yarn.lock # elk-ror/docker-compose.yml

coutoPL · 2024-09-30T15:10:58Z

.github/workflows/trigger-e2e-tests.yml

-#           organization: beshu-tech
-#           username: ${{ github.actor }}
-#           token: ${{ secrets.GITHUB_TOKEN }}
+  pull_request:


I see changes from my branch. For sure sth is wrong here.

coutoPL · 2024-09-30T15:13:50Z

e2e-tests/cypress/support/page-objects/invoices.ts

+    items.forEach((item: any) => {
+      const createResult = item.create;
+      if (createResult.status === 201) {
+        console.log(`Invoice ${createResult._id} created successfully.`);


do we have to add it? It spams the console with not relevant information

coutoPL · 2024-09-30T15:15:55Z

e2e-tests/cypress/e2e/Report-generation.cy.ts

+
+
+  before(() => {
+    dataPut(1000000, indexName, 600, 200);


"dataPut" doesn't tell much about what it does

coutoPL · 2024-09-30T15:16:24Z

e2e-tests/cypress/support/page-objects/invoices.ts

+
+export async function dataPut(count: number, indexName: string, chunkSize: number, threads: number): Promise<void> {
+  count = count / 2;
+  if (typeof cy.kbnPut !== 'function') {


why we have to check it?

coutoPL · 2024-09-30T15:18:46Z

e2e-tests/cypress/support/page-objects/invoices.ts

@@ -0,0 +1,177 @@
+// invoices.ts


we have SampleData class. Maybe we should add a function that generates invoices in some index there?

coutoPL · 2024-09-30T15:19:52Z

e2e-tests/cypress/support/page-objects/invoices.ts

+  });
+}
+
+function createIndex(indexName: string): Cypress.Chainable<void> {


ES API endpoint Typescript-cypress wrappers should be added in EsApiClient.ts

coutoPL · 2024-09-30T15:21:06Z

e2e-tests/cypress/support/page-objects/invoices.ts

+  }).then((response: any) => {
+    console.log('Response:', response);
+    if (response) {
+      if (response.error && response.error.type === 'resource_already_exists_exception') {


there is no need to do this. We should just assert that the response returned 2xx.

coutoPL · 2024-09-30T15:21:24Z

e2e-tests/cypress/support/page-objects/invoices.ts

+  const chunks = chunkArray(invoices, chunkSize);
+
+  const processChunk = async (chunk: any[], index: number) => {
+    cy.esPost({


the same - should be moved to EsApiClient

coutoPL · 2024-09-30T15:25:11Z

e2e-tests/cypress/e2e/Report-generation.cy.ts

+import { Reports } from "../support/page-objects/management/Reports";
+import { dataPut } from "../support/page-objects/invoices";
+
+describe('Report Generation', () => {


I ran the test - it's way too SLOOOOW.
Do we need so much data?

lodyczekoladowe and others added 3 commits August 10, 2024 23:00

test

c908b9b

Added Report-generation test

6833d96

Merge branch 'beshu-tech:master' into master

85faee9

lodyczekoladowe marked this pull request as draft August 14, 2024 21:35

lodyczekoladowe marked this pull request as ready for review August 14, 2024 21:36

lodyczekoladowe closed this Aug 14, 2024

lodyczekoladowe reopened this Aug 14, 2024

coutoPL requested changes Aug 15, 2024

View reviewed changes

coutoPL requested review from Dzuming and coutoPL August 15, 2024 09:10

coutoPL reviewed Aug 15, 2024

View reviewed changes

Dzuming requested changes Aug 16, 2024

View reviewed changes

lodyczekoladowe and others added 4 commits August 16, 2024 16:57

Update e2e-tests/cypress/e2e/Report-generation.cy.ts

5af8958

Update elk-ror/conf/kbn/kibana.yml

bbe787a

Part of the requested changes

5a1f6b2

Merge branch 'beshu-tech:master' into master

d595b45

lodyczekoladowe requested review from coutoPL and Dzuming August 29, 2024 13:14

All of requested changes. Waiting for pull request

e21ef59

coutoPL requested changes Sep 2, 2024

View reviewed changes

coutoPL reviewed Sep 2, 2024

View reviewed changes

Dzuming requested changes Sep 2, 2024

View reviewed changes

lodyczekoladowe and others added 5 commits September 5, 2024 00:34

Self-generate invoices data, checking report generation process..

4a01358

Merge branch 'beshu-tech:master' into master

3622e05

Pipeline fix and logging improvement (beshu-tech#6)

b8ad596

Sync 8.15.0 changes (beshu-tech#9)

0b75e9a

Async rapport generating update

879820b

lodyczekoladowe and others added 10 commits September 21, 2024 20:34

Fork sync

c2e51a0

Added Report-generation test

7a24af6

Update e2e-tests/cypress/e2e/Report-generation.cy.ts

a031977

Update elk-ror/conf/kbn/kibana.yml

d693e38

Part of the requested changes

0b1cb86

All of requested changes. Waiting for pull request

d2aa9ba

Self-generate invoices data, checking report generation process..

0fe0e10

Async rapport generating update

927f010

Fixes for new version of es

49bfd44

coutoPL requested changes Sep 30, 2024

View reviewed changes

coutoPL reviewed Sep 30, 2024

View reviewed changes

ROR_INTR_001 #3

Are you sure you want to change the base?

ROR_INTR_001 #3

Conversation

lodyczekoladowe commented Aug 14, 2024 • edited by coutoPL Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cypress tests in interactive GUI

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dzuming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dzuming commented Aug 30, 2024

lodyczekoladowe commented Aug 30, 2024

lodyczekoladowe commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lodyczekoladowe commented Aug 14, 2024 •

edited by coutoPL

Loading