Migrate query enhancement cypress tests to OSD repo #8986

abbyhu2000 · 2024-11-27T23:11:25Z

Description

Migrate query enhancement cypress tests to OSD repo.

Issues Resolved

Screenshot

Testing the changes

Changelog

feat: Migrate query enhancement cypress tests to OSD repo

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: abbyhu2000 <[email protected]>

github-actions · 2024-11-27T23:11:59Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

codecov · 2024-11-27T23:26:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (cypress-unification@d7b9046). Learn more about missing BASE report.

Additional details and impacted files

@@                  Coverage Diff                   @@
##             cypress-unification    #8986   +/-   ##
======================================================
  Coverage                       ?   60.89%           
======================================================
  Files                          ?     3802           
  Lines                          ?    91070           
  Branches                       ?    14374           
======================================================
  Hits                           ?    55456           
  Misses                         ?    32073           
  Partials                       ?     3541

Flag	Coverage Δ
Linux_1	`29.02% <ø> (?)`
Linux_2	`56.39% <ø> (?)`
Linux_3	`37.90% <ø> (?)`
Linux_4	`29.00% <ø> (?)`
Windows_1	`29.05% <ø> (?)`
Windows_2	`56.34% <ø> (?)`
Windows_3	`37.90% <ø> (?)`
Windows_4	`29.00% <ø> (?)`

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.

ananzh · 2024-11-28T00:52:09Z

cypress/utils/commands.js

@@ -2,6 +2,21 @@
 * Copyright OpenSearch Contributors
 * SPDX-License-Identifier: Apache-2.0
 */
+/*


duplicate license

ananzh · 2024-11-28T01:01:17Z

cypress/utils/dashboards/query_enhancement/commands.js

+ */
+
+Cypress.Commands.add('waitForLoaderNewHeader', () => {
+  const opts = { log: false };


I think for utilities, we need to be careful. Eventually we will move the legacy discover ftr tests back to OSD as well. So the utilities should work for both legacy and is_enhancement is true. For example if could could have both of them working, that would save us a lot of future migration efforts

Cypress.Commands.add('waitForLoader', (isEnhancement = false) => { const opts = { log: false }; Cypress.log({ name: 'waitForPageLoad', displayName: 'wait', message: 'page load', }); cy.wait(Cypress.env('WAIT_FOR_LOADER_BUFFER_MS')); // Use recentItemsSectionButton for query enhancement, otherwise use homeIcon cy.getElementByTestId( isEnhancement ? 'recentItemsSectionButton' : 'homeIcon', opts ); });

ananzh · 2024-11-28T01:03:18Z

Should we also add auth utilities and workspace utilities. Not blocker and can do these in the follow up.

kavilla · 2024-11-28T01:20:09Z

just to verify what is the goal for this pr? is it just to get it to this repo right? if so then i'm going to leave comments with the implication that we should fast follow these things but NOT block on anything

kavilla · 2024-11-28T01:29:48Z

cypress/fixtures/dashboard/opensearch_dashboards/data.json.txt

+    "id": "3",
+    "index": "timestamp-nanos",
+    "source": {
+      "timestamp": "2019-01-01T12:10:30.123456789Z"


we should make the file name of this not data.json.txt since that is too generic of a name and could cause confusion.

we should also have more realistic data here as in some other fields maybe even number of other timefields. the size should be more than two. this way the test results for casting a wide net of a date range will yield 2 results the same way filtering by the jan 1st of 2019 would which could maybe lead to false positives in our filtering tests

kavilla · 2024-11-28T01:31:30Z

cypress/fixtures/dashboard/opensearch_dashboards/data_with_index_pattern.json.txt

+{
+  "type": "doc",
+  "value": {
+    "id": "index-pattern:timestamp-*",


is this the behavior we wanna continue? i think we did this from the legacy but if we we want to add more documents to these tests to capture the cases we are running into when regressions occur

if not then we should just consider having stored the ndjson of saved objects and then we just upload them on test start.

this way if we have massive test data we dont know need to open or modify that file to modify the saved objects

kavilla · 2024-11-28T01:36:38Z

package.json

@@ -84,8 +84,9 @@
    "spec_to_console": "scripts/use_node scripts/spec_to_console",
    "pkg-version": "scripts/use_node -e \"console.log(require('./package.json').version)\"",
    "release_note:generate": "scripts/use_node scripts/generate_release_note",
-    "cypress:run-without-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --headless --env SECURITY_ENABLED=false",
-    "cypress:run-with-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --headless --env SECURITY_ENABLED=true,openSearchUrl=https://localhost:9200,WAIT_FOR_LOADER_BUFFER_MS=500",
+    "cypress:run-without-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --env SECURITY_ENABLED=false",


follow up:

run-without-security is long to type out

we should upate the appropriate places and here so like

cypress:run // implies it is without security cypress:run:security // explicit with security

this follows the above structure yarn start:security

also might need to mention in some readme to run this function with --headless if people want to get the runner open

kavilla · 2024-11-28T01:40:54Z

package.json

-    "cypress:run-with-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --headless --env SECURITY_ENABLED=true,openSearchUrl=https://localhost:9200,WAIT_FOR_LOADER_BUFFER_MS=500",
+    "cypress:run-without-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --env SECURITY_ENABLED=false",
+    "cypress:run-with-security": "env TZ=America/Los_Angeles NO_COLOR=1 cypress run --env SECURITY_ENABLED=true,openSearchUrl=https://localhost:9200,WAIT_FOR_LOADER_BUFFER_MS=500",
+    "osd:ciGroup10": "echo \"apps/query_enhancement/*.js\"",


sync with @d-rowe . and def not a blocker for any of this but we should consider while in theprocess of migrating defining a middle ground solution at least for the tests to tag themselves instead of a package.json file. because this was only suppose to be temporary and people wont know they need to update thisfor the test to run

something like the selenium stuff https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/test/functional/apps/dashboard/index.js#L83 but even a hackier one that is not using this hack would be better imo

like i said not blocking just mentioning incase it would make the work smaller increments

kavilla · 2024-11-28T01:41:42Z

cypress/utils/index.d.ts

+     * @example
+     * cy.whenTestIdNotFound(['query', 'puery'], () => {...})
+     */
+    whenTestIdNotFound<S = any>(


@d-rowe @ananzh so it seems we are already migrating from that package in here too right?

also do people call this?

kavilla · 2024-11-28T01:44:15Z

cypress/utils/index.d.ts

+     * cy.getElementsByTestIds(['query', 'puery'])
+     */
+    getElementsByTestIds<S = any>(
+      testIds: string | string[],


ik carry over but too flexiable of a system is hard to keep up. i think if we have a getelementbytestid then it should just be string[] here

kavilla · 2024-11-28T01:44:56Z

cypress/utils/index.d.ts

+    bulkUploadDocs<S = any>(
+      fixturePath: string,
+      index: string
+      // options?: Partial<Loggable & Timeoutable & Withinable & Shadow>


shoudl this overwrite?

kavilla · 2024-11-28T01:46:31Z

cypress/utils/dashboards/query_enhancement/constants.js

+ */
+
+export const clusterName = 'test_cluster';
+export const clusterConnection = 'http://localhost:9200';


lets not port this over imo. this should pull from a config and if that config is suppose to point to an external url then only the cypress config would be able to update so easily this constant is a constant and wont be reusable in cases

kavilla · 2024-11-28T01:47:43Z

cypress/utils/dashboards/query_enhancement/commands.js

+  });
+
+  cy.getElementByTestId(`queryEditorLanguageSelector`).click();
+  cy.get(`[class~="languageSelector__menuItem"]`).contains(value).click({


we could either add a test id which might be easier or access this class from the parent that has a test id of queryeditor langauge selector to ensure it doesn't grab the wrong language selector

kavilla · 2024-11-28T01:48:58Z

cypress/utils/helpers.js

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export const apiRequest = (url, method = 'POST', body = undefined, qs = undefined) =>


we should make this an object after url and method so it wont get so difficutl to update if we need to add more

kavilla · 2024-11-28T01:49:57Z

cypress/utils/helpers.js

+export const devToolsRequest = (url, method = 'POST', body = undefined, qs = undefined) =>
+  cy.request({
+    method: 'POST',
+    form: false,


nit: seems like there could be a request headers constant that we can spread here

kavilla · 2024-11-28T01:53:22Z

cypress/utils/helpers.js

+    qs: qs,
+  });
+
+export const devToolsRequest = (url, method = 'POST', body = undefined, qs = undefined) =>


nit: this is kind of the wrong name. i think it's technically proxying to the console plugin dev tools is an application that is a child of the console plugin but it is client side.

so we aren't actually firing the request to the ui we are hitting the node server api

kavilla · 2024-11-28T01:54:23Z

cypress/utils/dashboards/constants.js

+export const INDEX_PATTERN_PATH = STACK_MANAGEMENT_PATH + '/opensearch-dashboards/indexPatterns';
+export const SAVED_OBJECTS_PATH = STACK_MANAGEMENT_PATH + '/opensearch-dashboards/objects';
+
+export * from './query_enhancement/constants';


we shouldn't export another folders constants here

kavilla · 2024-11-28T01:55:31Z

cypress/utils/dashboards/commands.js

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import './query_enhancement/commands';


we should have global commands if needed. and then query enhancmeents and dashboards can take from it vs another plugin command

kavilla · 2024-11-28T01:56:14Z

cypress/utils/commands.js

+  cy.log('Deleting all indices');
+  cy.request(
+    'DELETE',
+    `${Cypress.env('openSearchUrl')}/index*,sample*,opensearch_dashboards*,test*,cypress*`


do we ahve to refresh?

kavilla · 2024-11-28T01:56:32Z

cypress/utils/commands.js

+
+import { BASE_PATH } from './constants';
+
+// This function does not delete all indices


can we modify this now to actually delete all indices and not a specific string?

kavilla · 2024-11-28T01:57:51Z

cypress/utils/commands.js

+  cy.log('Deleting all indices');
+  cy.request(
+    'DELETE',
+    `${Cypress.env('openSearchUrl')}/index*,sample*,opensearch_dashboards*,test*,cypress*`


now that we have it can we just say ENGINE_URL anything besides the casing

kavilla · 2024-11-28T01:58:43Z

cypress/utils/commands.js

+    method: 'PUT',
+    url,
+    headers: {
+      'content-type': 'application/json;charset=UTF-8',


we can modify the cy.request which i think the security plugin commands are doing to just add some of these copy and paste request headers

kavilla · 2024-11-28T02:01:30Z

cypress/utils/dashboards/data_explorer/constants.js

+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+export const DE_INDEX_DATA = '';


empty?

also instead of us needing to import all these individual sometime we can namespace stuff like

DE: { INDEX: { ID: 'data-explorer', DATA: '', } ...

so then we would have to import 3 things we just import one and use dot notation

simplify the importing using namespace, for example:

export const PATHS = { BASE: BASE_PATH, ENGINE: BASE_ENGINE.url, SECONDARY_ENGINE: SECONDARY_ENGINE.url, STACK_MANAGEMENT: BASE_PATH + '/app/management', SECURITY_PLUGIN: BASE_PATH + '/app/security-dashboards-plugin#/', TENANTS_MANAGE: BASE_PATH + '/app/security-dashboards-plugin#/tenants', INDEX_PATTERNS: BASE_PATH + '/app/management/opensearch-dashboards/indexPatterns', SAVED_OBJECTS: BASE_PATH + '/app/management/opensearch-dashboards/objects', };

kavilla · 2024-11-28T02:02:04Z

cypress/utils/dashboards/data_explorer/constants.js

+
+export const DE_PATH_FIXTURE = 'dashboard/opensearch_dashboards/data_explorer/';
+
+export const DE_DEFAULT_START_TIME = 'Sep 19, 2015 @ 06:31:44.000';


this is a good candidate. for the above comment too but

why not have this higher up or not global defined?

kavilla · 2024-11-28T02:03:25Z

cypress/integration/core_opensearch_dashboards/apps/query_enhancement/dataset_selector_spec.js

do we have to keep this folder structure?

core_opensearch_dashboards is redudant now since we are in the opensearch dashboards repo. we can reduce the messiness now since we should only be our tests for our plugins

@d-rowe @abbyhu2000 @ananzh did you guys before porting over want to list out what is the structure of the nested folders we should shoot for? otherwise we will just port of tech debt that is just really busy work. unless this was a drop in replacement why would be suprrising then probably we are spending the effort to keep the tech debt.

we should just quickly list out what we want the end state to kind of look like

Made the file structure a little nicer by getting rid of /core-opensearch-dashboard and /plugin

EX:
/cypress
/integration
/apps
/query_enhancements
/test files
/another_plugin
/test files

kavilla · 2024-11-28T02:05:33Z

cypress/integration/core_opensearch_dashboards/apps/query_enhancement/dataset_selector_spec.js

ide would be able to let us know what is a .spec file but dataset_selector_spec.js would just seem like a regular file. so please use dataset_selector.spec.js

also why not typescript? @d-rowe is this setup for tyupescript?

Yup, this package is setup for typescript. It'd be great to have these be typescript files, but actually backfilling the types would be quite an effort during this migration it seems.

kavilla · 2024-11-28T02:12:49Z

cypress/integration/core_opensearch_dashboards/apps/query_enhancement/dataset_selector_spec.js

+  describe('select indices', () => {
+    before(() => {
+      testFixtureHandler.importJSONMapping(
+        'cypress/fixtures/dashboard/opensearch_dashboards/query_enhancement/mappings.json.txt'


we could consider a utils function that does this a little bit easier like we know we want to store in fixtures so then we just need to have the plugin define it data it wants to use.

but i dont really see why each plugin needs to define its own fixtures.

if we add a new plugin then the plugin will think it needs to keep adding more sample data that is increasing the resources taken for local development when this data could have been re-used

agree on getting a utils function so importing datas can be cleaner.

Currently changing the fixture file structure so that dashboard and plugin fixtures are not seperated, instead each folder are the data name and contain one data file and one mapping file.

Ex:

/fixtures
/discover
/data.json
/mappings.json
/logstash
/data.json
/mappings.json
/timestamp
/data.json
/mappings.json
.....

abbyhu2000 · 2024-12-14T00:23:09Z

close this in favor of #9048

Migrate query enhancement cypress tests to OSD

e28133c

Signed-off-by: abbyhu2000 <[email protected]>

github-actions bot added the distinguished-contributor label Nov 27, 2024

github-actions bot added the failed changeset label Nov 27, 2024

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

f42d1dd

github-actions bot removed the failed changeset label Nov 27, 2024

ananzh reviewed Nov 28, 2024

View reviewed changes

kavilla reviewed Nov 28, 2024

View reviewed changes

abbyhu2000 mentioned this pull request Dec 12, 2024

Migrate query enhancement tests from functional repo to main #9048

Merged

7 tasks

abbyhu2000 closed this Dec 14, 2024


		import { BASE_PATH } from './constants';

		// This function does not delete all indices


		export const DE_PATH_FIXTURE = 'dashboard/opensearch_dashboards/data_explorer/';

		export const DE_DEFAULT_START_TIME = 'Sep 19, 2015 @ 06:31:44.000';

Migrate query enhancement cypress tests to OSD repo #8986

Migrate query enhancement cypress tests to OSD repo #8986

Conversation

abbyhu2000 commented Nov 27, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Nov 27, 2024

❌ Empty Changelog Section

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ananzh commented Nov 28, 2024

kavilla commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla Nov 28, 2024 • 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

Choose a reason for hiding this comment

kavilla Nov 28, 2024 • 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

abbyhu2000 commented Dec 14, 2024

abbyhu2000 commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

kavilla Nov 28, 2024 •

edited

Loading

kavilla Nov 28, 2024 •

edited

Loading