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 functional test for UI Metric #1223

Closed
wants to merge 0 commits into from

Conversation

LDrago27
Copy link
Contributor

@LDrago27 LDrago27 commented Apr 22, 2024

Description

Add functional tests to validate the UI Metric Collector introduced in OpenSearch dashboards core.

Tests

ui_metric_no_security.spec.js.mp4
ui_metric_with_security.spec.js.mp4

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.


miscUtils.visitPage('app/home#/');

cy.wait(45000);
Copy link
Member

Choose a reason for hiding this comment

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

Lets not add wait statements to the test. Cant we wait for the loader? There should be a helper function to do this. You can do something similar for the other waits as well

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the tests to remove wait statements. Instead we are now waiting for the API call to be successful. @ashwin-pc @kavilla


cy.request(
'GET',
'http://localhost:5601/api/stats?extended=true&legacy=true&exclude_usage=false'
Copy link
Member

Choose a reason for hiding this comment

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

pull from cypress settings the dashboards url.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kavilla Update to the use base path from config.

@kavilla
Copy link
Member

kavilla commented Apr 26, 2024

Not necessarily a blocker but I would also add a new setting to the config: https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/cypress.json#L26.

Then we can actually enable this and disable the running of these tests. Especially if it's default disabled then likely the tests will fail if they are executed.

Here's an example:
https://github.com/search?q=repo%3Aopensearch-project%2Fopensearch-dashboards-functional-test%20ML_COMMONS_DASHBOARDS_ENABLED&type=code

edit: Yeah I think it should be a blocker if ui metric is disabled by default to avoid test failures.

@LDrago27
Copy link
Contributor Author

@kavilla that sounds good. I can update the tests so that they are being guarded by the config.

@kavilla
Copy link
Member

kavilla commented Apr 26, 2024

@kavilla
Copy link
Member

kavilla commented Apr 26, 2024

Note for after merging. If you would for these tests to then be executed from the build CI then we will need to update the test manifest:
https://github.com/opensearch-project/opensearch-build/blob/main/manifests/2.14.0/opensearch-dashboards-2.14.0-test.yml#L14

@kavilla
Copy link
Member

kavilla commented Apr 26, 2024

Also for after merging updating OSD too if you would like it to be ran in our GitHub ci: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/.github/workflows/cypress_workflow.yml#L37. But might have to be cautious about that. We have been hitting water marks for memory so depends on how much memory UI metric consumes.

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.

4 participants