[Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) #9166

ArgusLi · 2025-01-09T18:17:48Z

Description

Adds a Cypress test suite for filtering in the Discover page.

Issues Resolved

Closes #8976.

Screenshot

newUICompressed.mp4

oldUICompressed.mp4

Testing the changes

With OSD running, run yarn run cypress open. In E2E specs, you will see 2 new test specs saved_queries.spec.js and saved_queries_old_ui.spec.js. Run saved_queries.spec.js if data.savedQueriesNewUI.enabled: true in the OSD config, else run saved_queries_old_ui.spec.js.

Changelog

test: Add cypress integration test for the old and new UI view saved queries.

Signed-off-by: Argus Li <[email protected]>

...rch-dashboards/opensearch-dashboards/apps/query_enhancements/field_display_filtering.spec.js

angle943 · 2025-01-10T19:42:16Z

...re-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_queries.spec.js

+        cy.saveQuery(config.saveName);
+      });
+    });
+    it('should see all saved queries', () => {


do we need ... for ${config.testName}?

EDIT: Nevermind, it looks like this it is not part of the testConfigurations.forEach(). If so could we add a blank line between 87 and 88 just to make that a bit cle4ar (nitpick comment)

angle943 · 2025-01-10T19:42:38Z

...re-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_queries.spec.js

+    });
+    it('should see all saved queries', () => {
+      cy.getElementByTestId('saved-query-management-popover-button').click({
+        force: true,


why is force: true needed here?

I had some issues where sometimes just clicking wouldn't open the popover. I will add a comment here to explain.

if it was not working but the element not obstructed, i wonder if this works:

cy.getElementByTestId('saved-query-management-popover-button').should('be.visible').click()

(This will ensure it is visible in the DOM before clicking. i wonder if you are attempting to click it before it is visible)

I tried again without the force and it's working now. I think this one leftover from when I was trying to debug an issue. Removing the force.

angle943 · 2025-01-10T19:44:33Z

...re-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_queries.spec.js

+      cy.getElementByTestId('saved-query-management-open-button').click();
+
+      //Wait for queries to load
+      cy.wait(2000);


instead of arbitrary 2000, could we wait for some element to be loaded?

This was left over from when I copied your structure for saved search. It doesn't seem needed for saved queries. Removing.

angle943 · 2025-01-10T19:45:38Z

...search-dashboards/opensearch-dashboards/apps/query_enhancements/saved_queries_old_ui.spec.js

@@ -0,0 +1,105 @@
+/*


I don't have enough context here, so would love for @ananzh or @LDrago27 's response, but for old_ui, does file need to be in a separate folder?

It's a specific flag for saved_queries specifically, my first thought is that it makes sense to just separate the files based on the ui version for now, but if there are any other specs that use the same flag, then putting them into separate folders would be a good idea.

i think we split up the running of tests with different configs based on folder structure, so it might be problematic for this to be in the same folder as the new-ui. but again i'll defer to @LDrago27 and @ananzh

...re-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/saved_queries.spec.js

angle943 · 2025-01-10T19:49:15Z

...e-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/utils/saved_queries.js

+    default:
+      throw new Error(`getQueryString encountered unsupported language: ${language}`);
+  }
+};


(I can do this)

But it looks like this and some other functions are exactly the same as mine. I will make some refactor in the future to put them in shared.

I've separated this function because I will be needing to make some changes to it and its associated functions for the specific queries in following saved queries tests.

@ArgusLi Lets move all the utils inside to this path /cypress/utils/apps/query_enhancements. That will help us to consolidate the utils method in one place.

angle943 · 2025-01-10T19:50:21Z

cypress/utils/apps/data_explorer/commands.js

@@ -125,33 +125,49 @@ Cypress.Commands.add(
  }
 );

-Cypress.Commands.add('saveQuery', (name, description) => {
+Cypress.Commands.add('saveQueryOldUI', (name, description = ' ') => {


can we rename this to something else? I dont' have enough context, but what constitutes a 'new ui' vs old ui? I don't like OldUi because in a few months/years, what we consider now to be newUi will become oldUi

The flag is data.savedQueriesNewUI.enabled: false, which is why I named it as such. I agree though. Would love to discuss this offline with @LDrago27.

angle943 · 2025-01-10T19:50:32Z

cypress/utils/apps/data_explorer/commands.js

  cy.whenTestIdNotFound('saved-query-management-popover', () => {
    cy.getElementByTestId('saved-query-management-popover-button').click();
  });
  cy.getElementByTestId('saved-query-management-save-button').click();

  cy.getElementByTestId('saveQueryFormTitle').type(name);
  cy.getElementByTestId('saveQueryFormDescription').type(description);
+
+  cy.getElementByTestId('savedQueryFormSaveButton').click({ force: true });


why is force needed? Typically, this is only needed if the element is hidden by some other element in the UI (like a popover). if so, then it is valid that it is needed but a comment should be there signifying why it's needed. Else we can just get rid of force: true

I think this is actually left over from some testing. Will reverify and check.

Confirmed. Removing.

angle943 · 2025-01-10T19:51:37Z

cypress/utils/apps/data_explorer/commands.js

+  cy.getElementByTestId('saveQueryFormTitle').type(name);
+  cy.getElementByTestId('saveQueryFormDescription').type(description);
+
+  cy.getElementByTestId('savedQueryFormSaveButton').click({ force: true });


same forc

I think this is actually left over from some testing. Will reverify and check.

Confirmed, removing.

angle943 · 2025-01-10T19:52:52Z

cypress/utils/apps/query_enhancements/commands.js

-  cy.getElementByTestId('editDatasourceDeleteIcon').click();
-  cy.getElementByTestId('confirmModalConfirmButton').click();
+  cy.getElementByTestId('editDatasourceDeleteIcon').click({ force: true });
+  cy.getElementByTestId('confirmModalConfirmButton').click({ force: true });


if we remove deleteDataSourceByName from the other places, these changes don't need to be here. But IF i'm wrong and deleteDataSourceByName needs to be there, two things:

lets get rid of cy.wait(2000) and replace it with waiting for some element to exist.

Lets get rid of force: true unless its really needed (and if so, add a comment why)

Let me try again. The reason why this was here is because of some really weird behaviour from Cypress. The deleteSourceByName was initially in the afterEach block, but I changed it to the after block instead. When it was in the afterEach block, when the first saved query was created, it executed perfectly, but would fail the next time it was run after the next saved query was created.

I tried getting the element without a chained click, which would lead to Cypress' timeout and retry logic, but for some reason it still failed. I tried using waitForLoader but that didn't work either, which is when I shifted to the wait. But after that, it still failed when clicking the editDatasourceDeleteIcon button - the button was selected, and was clicked, but no action actually happened. Which is why I had to use force: true. The same thing happened for confirmModalConfirmButton.

But none of these will happen now given it is only run once in the after block so I'll try and see if removing them will work now.

Yup, because it's only run once now, it works without these forces and waits. Removing the wait and forces.

Signed-off-by: Argus Li <[email protected]>

codecov · 2025-01-10T21:02:08Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.01%. Comparing base (82689bb) to head (b411dd0).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...lugins/data/public/ui/saved_query_form/helpers.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9166   +/-   ##
=======================================
  Coverage   61.01%   61.01%           
=======================================
  Files        3812     3813    +1     
  Lines       91385    91396   +11     
  Branches    14438    14439    +1     
=======================================
+ Hits        55762    55769    +7     
- Misses      32065    32068    +3     
- Partials     3558     3559    +1

Flag	Coverage Δ
Linux_1	`29.08% <ø> (+<0.01%)`	⬆️
Linux_2	`56.45% <ø> (ø)`
Linux_3	`38.02% <0.00%> (-0.01%)`	⬇️
Linux_4	`29.03% <ø> (ø)`
Windows_1	`29.09% <ø> (+<0.01%)`	⬆️
Windows_2	`56.40% <ø> (ø)`
Windows_3	`38.03% <0.00%> (+<0.01%)`	⬆️
Windows_4	`29.03% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cypress/utils/apps/query_enhancements/commands.js

Signed-off-by: Argus Li <[email protected]>

LDrago27 · 2025-01-10T23:52:23Z

...e-opensearch-dashboards/opensearch-dashboards/apps/query_enhancements/utils/saved_queries.js

+  }
+};
+
+/**


nit: Update the comments to indicate it is for saved queries instead of saved search

Signed-off-by: Argus Li <[email protected]>

ArgusLi added 7 commits January 8, 2025 21:09

add spec for old ui

02bb7d4

Signed-off-by: Argus Li <[email protected]>

Add comment about cleaning up saved queries

af1352d

Signed-off-by: Argus Li <[email protected]>

Add data-test-subj for save query new ui

d6a713b

Signed-off-by: Argus Li <[email protected]>

Add data-test-subj for save query button for new ui

d6e0785

Signed-off-by: Argus Li <[email protected]>

Add data-test-subj for open query button in query management popover

a15bba2

Signed-off-by: Argus Li <[email protected]>

Add new ui spec to test list saved queries

47fe9a7

Signed-off-by: Argus Li <[email protected]>

Fix DataSource url

49a9352

Signed-off-by: Argus Li <[email protected]>

ArgusLi requested a review from huyaboo as a code owner January 9, 2025 18:17

github-actions bot added the repeat-contributor label Jan 9, 2025

ArgusLi and others added 2 commits January 9, 2025 10:18

Merge branch 'main' into testid-124-list-saved-queries

61112c8

Changeset file for PR opensearch-project#9166 created/updated

00b3f60

ArgusLi changed the title ~~[Discover 2.0 Testing] Create View Saved Queries Test (Test-Id 124: List Saved Queries)~~ [Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) Jan 9, 2025

angle943 reviewed Jan 10, 2025

View reviewed changes

...rch-dashboards/opensearch-dashboards/apps/query_enhancements/field_display_filtering.spec.js Show resolved Hide resolved

angle943 suggested changes Jan 10, 2025

View reviewed changes

Address Justin's comments.

edb8f9f

Signed-off-by: Argus Li <[email protected]>

angle943 reviewed Jan 10, 2025

View reviewed changes

cypress/utils/apps/query_enhancements/commands.js Outdated Show resolved Hide resolved

Remove final force.

4ae05a4

Signed-off-by: Argus Li <[email protected]>

LDrago27 reviewed Jan 10, 2025

View reviewed changes

Fix final force.

b411dd0

Signed-off-by: Argus Li <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) #9166

[Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) #9166

ArgusLi commented Jan 9, 2025 •

edited

Loading

angle943 Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025 •

edited

Loading

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

LDrago27 Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

ArgusLi Jan 10, 2025

angle943 Jan 10, 2025

ArgusLi Jan 10, 2025

ArgusLi Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

LDrago27 Jan 10, 2025

+                }
+              };
+              /**

[Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) #9166

Are you sure you want to change the base?

[Discover 2.0 Testing] Create View List of Saved Queries Test (Test-Id 124: View list of Saved query) #9166

Conversation

ArgusLi commented Jan 9, 2025 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

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

ArgusLi Jan 10, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArgusLi Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ArgusLi commented Jan 9, 2025 •

edited

Loading

ArgusLi Jan 10, 2025 •

edited

Loading

ArgusLi Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading