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

Add cypress tests for discover 2.0 #1598

Merged
merged 9 commits into from
Nov 14, 2024

Conversation

abbyhu2000
Copy link
Member

Description

Add two test files to test on discover 2.0 functionalities.

  • queries
  • dataset navigator

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Overall i love the tests. A few coments and concerns. Let me know what you think of them

Comment on lines 77 to 80
cy.getElementByTestId('queryEditorLanguageSelector').should(
'contain',
'SQL'
);
Copy link
Member

Choose a reason for hiding this comment

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

This will be flakey unless we can set this before hand right?

Comment on lines 82 to 85
// The following steps are needed because when selecting SQL, discover loaded with data but the
// multi-line query editor are not loaded properly(it renders a single line query bar) unless we select SQL again
// This bug only exist in cypress test; can not reproduce manually
cy.get(`[data-test-subj="queryEditorLanguageSelector"]`).click();
Copy link
Member

Choose a reason for hiding this comment

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

Will a wait help?

`app/data-explorer/discover#/?_g=(filters:!(),time:(from:'2015-09-19T13:31:44.000Z',to:'2015-09-24T01:31:44.000Z'))`
);

cy.get(`[class~="datasetSelector__button"]`).click();
Copy link
Member

Choose a reason for hiding this comment

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

If there are index patterns, the default index pattern should be selected by default. We should not have to select it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea when i tried it manually, the default index pattern will be picked up by default; but somehow in the test, it wont. @ashwin-pc

Copy link
Member

Choose a reason for hiding this comment

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

I also saw this issue. I think due to double render, first select as the default index then render. So in cypress we will capture this and need to re-select it. Also saw SuperDatePicker detached from the DOM due to the same reason. We will document any issues due to render or async.

Copy link
Member

Choose a reason for hiding this comment

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

is this still the case

Copy link
Member

Choose a reason for hiding this comment

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

yes

@@ -34,6 +34,7 @@
"osd:ciGroup7": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/doc_navigation.spec.js,apps/data_explorer/doc_table.spec.js,apps/data_explorer/errors.spec.js,apps/data_explorer/field_data.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup8": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/field_visualize.spec.js,apps/data_explorer/filter_editor.spec.js,apps/data_explorer/index_pattern_with_encoded_id.spec.js,apps/data_explorer/index_pattern_without_field.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup9": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/inspector.spec.js,apps/data_explorer/large_string.spec.js,apps/data_explorer/saved_queries.spec.js,apps/data_explorer/shared_links.spec.js,apps/data_explorer/sidebar.spec.js,apps/data_explorer/source_filter.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup10": "echo apps/query_enhancement/*.js",
Copy link
Member

Choose a reason for hiding this comment

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

Wont this require all the YAML settings and Advanced settings for this view to be set correctly?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you mean by this?

Copy link
Member

Choose a reason for hiding this comment

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

that's a good call out @ashwin-pc

@abbyhu2000 we start query enhancements with an advanced setting overridden. but doesn't mean everyone will know that.

we should:

  • part of a test setup is to set the query enhancements advanced setting to true.
  • check if we can re-use some of the MDS test setup. because right now i dont think the tests would be able to much besides things like index patterns as we move the default cluster option and i dont know if the test sets up a call to an external cluster. so you wont be able to select an index. so maybe we can set up on the ci two clusters starting up so that we can add that data soruce as an external data source. this will mean we will need to handle the situation when we start with MDS or not.

it('with SQL as default language', function () {
cy.getElementByTestId(`datasetSelectorButton`).click();
cy.getElementByTestId(`datasetSelectorAdvancedButton`).click();
cy.get(`[title="Indexes"]`).click();
Copy link
Member

Choose a reason for hiding this comment

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

could we add data test subjects to these?

@@ -34,6 +34,7 @@
"osd:ciGroup7": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/doc_navigation.spec.js,apps/data_explorer/doc_table.spec.js,apps/data_explorer/errors.spec.js,apps/data_explorer/field_data.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup8": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/field_visualize.spec.js,apps/data_explorer/filter_editor.spec.js,apps/data_explorer/index_pattern_with_encoded_id.spec.js,apps/data_explorer/index_pattern_without_field.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup9": "echo apps/data_explorer/aaa_before.spec.js,apps/data_explorer/inspector.spec.js,apps/data_explorer/large_string.spec.js,apps/data_explorer/saved_queries.spec.js,apps/data_explorer/shared_links.spec.js,apps/data_explorer/sidebar.spec.js,apps/data_explorer/source_filter.spec.js,apps/data_explorer/zzz_after.spec.js",
"osd:ciGroup10": "echo apps/query_enhancement/*.js",
Copy link
Member

Choose a reason for hiding this comment

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

that's a good call out @ashwin-pc

@abbyhu2000 we start query enhancements with an advanced setting overridden. but doesn't mean everyone will know that.

we should:

  • part of a test setup is to set the query enhancements advanced setting to true.
  • check if we can re-use some of the MDS test setup. because right now i dont think the tests would be able to much besides things like index patterns as we move the default cluster option and i dont know if the test sets up a call to an external cluster. so you wont be able to select an index. so maybe we can set up on the ci two clusters starting up so that we can add that data soruce as an external data source. this will mean we will need to handle the situation when we start with MDS or not.

Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
Signed-off-by: Qingyang(Abby) Hu <[email protected]>
@abbyhu2000
Copy link
Member Author

@Hailong-am Hailong-am merged commit c3abd79 into opensearch-project:main Nov 14, 2024
57 of 64 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 14, 2024
* add reload to PPL

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* Add initial query enhancement tests

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* Add ciGroup10

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* modify queries test

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* fix tests

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* fix one more test

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* address comments

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* remove deleteAll

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* delete data source connection

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

---------

Signed-off-by: Qingyang(Abby) Hu <[email protected]>
(cherry picked from commit c3abd79)
Hailong-am pushed a commit that referenced this pull request Nov 15, 2024
* add reload to PPL

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* Add initial query enhancement tests

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* Add ciGroup10

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* modify queries test

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* fix tests

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* fix one more test

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* address comments

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* remove deleteAll

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

* delete data source connection

Signed-off-by: Qingyang(Abby) Hu <[email protected]>

---------

Signed-off-by: Qingyang(Abby) Hu <[email protected]>
(cherry picked from commit c3abd79)

Co-authored-by: Qingyang(Abby) Hu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants