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

[RHOAIENG-18124] Add Workload Metrics Sanity tests #3767

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Srihari1192
Copy link

@Srihari1192 Srihari1192 commented Feb 17, 2025

https://issues.redhat.com/browse/RHOAIENG-18124

Description

  • Migrated Workload metrics sanity tests from ods-ci robot framework to cypress
  • Added Supporting methods for kueue resource creation, deletion and WorkloadMetricsTestData

How Has This Been Tested?

This change will include workloadmetrics Sanity tests, tested in local against rhoai nightly build
image

Test Impact

None - this is a test

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

Copy link
Contributor

openshift-ci bot commented Feb 17, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Feb 17, 2025
@Srihari1192 Srihari1192 force-pushed the addWorkloadMetricsTests branch 3 times, most recently from 6967da6 to 9885482 Compare February 17, 2025 10:34
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.64%. Comparing base (9b6b7a6) to head (3804c6c).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3767   +/-   ##
=======================================
  Coverage   84.63%   84.64%           
=======================================
  Files        1515     1515           
  Lines       35100    35100           
  Branches     9814     9814           
=======================================
+ Hits        29707    29709    +2     
+ Misses       5393     5391    -2     

see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b6b7a6...3804c6c. Read the comment docs.

@Srihari1192 Srihari1192 force-pushed the addWorkloadMetricsTests branch 3 times, most recently from 24138c7 to 484ffed Compare February 18, 2025 06:58
@Srihari1192 Srihari1192 marked this pull request as ready for review February 18, 2025 06:58
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Feb 18, 2025
Copy link
Contributor

@manosnoam manosnoam left a comment

Choose a reason for hiding this comment

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

Please rebase to use retryableBefore instead of before in your tests.
Also, no need to specify running with ODS-CI - we do not use it to run Cypress.

@Srihari1192 Srihari1192 force-pushed the addWorkloadMetricsTests branch 2 times, most recently from 2381c69 to a11acb1 Compare February 18, 2025 14:20
@Srihari1192 Srihari1192 force-pushed the addWorkloadMetricsTests branch 2 times, most recently from 93ac3e4 to 7e432e4 Compare February 19, 2025 04:10
Copy link
Contributor

@manosnoam manosnoam left a comment

Choose a reason for hiding this comment

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

/lgtm

@Srihari1192
Copy link
Author

Srihari1192 commented Feb 19, 2025

@antowaddle Can you review this PR

Copy link
Contributor

@antowaddle antowaddle left a comment

Choose a reason for hiding this comment

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

@Srihari1192 Thanks for the contribution, really appreciate new contributions to the framework. I've added some comments, can you review them please? Additionally, can i ask that you review #3614 and use this format on your PR also.

Copy link
Contributor

openshift-ci bot commented Feb 24, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from antowaddle. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Srihari1192 Srihari1192 force-pushed the addWorkloadMetricsTests branch from c982bbd to 3804c6c Compare February 25, 2025 06:22
@manosnoam manosnoam added the lgtm label Feb 25, 2025
@manosnoam manosnoam assigned Srihari1192 and unassigned manosnoam and antowaddle Feb 25, 2025
Copy link
Contributor

@antowaddle antowaddle left a comment

Choose a reason for hiding this comment

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

I ran this on an ODH cluster, it works successfully.
image

I've added 2 more minor comment, can you review them please?


// Get the tooltip element and extract its text
return cy
.get("[style*='fill: var(--pf-v6-chart-tooltip--Fill']")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we create a testID for the tooltip? We typically do not use patternfly locators in our test as they can change which will break the test and require maintenance.

})
.then(() => {
cy.log('Creating Namespace ${projectName}');
createOpenShiftProject(projectName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use createCleanProject here instead? Currently this test will fail if the project already exists on the cluster, createCleanProject will do a cleanup if the test exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants