-
Notifications
You must be signed in to change notification settings - Fork 162
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
Task/APPENG-2738: NIM Cypress E2E tests #3298
base: main
Are you sure you want to change the base?
Conversation
Hi @dmartinol. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
8480168
to
e8be6e5
Compare
Below check on the PR is currently failing, but it seems to be passing in our local environment. It might be worth re-running the check to determine if it's a legitimate issue. https://github.com/opendatahub-io/odh-dashboard/actions/runs/11182131290/job/31087701260?pr=3298 faling cypress file as per above check - on local it is successful |
/ok-to-test |
Signed-off-by: Tomer Figenblat <[email protected]>
…s standalone cypress test suite.
…#2) Signed-off-by: Daniele Martinoli <[email protected]>
…render nim and all other cards on overview and model tab when there are no ServingRuntimes configured.
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
* when combination of configurations to enable nim models are not configured properly.
Signed-off-by: Tomer Figenblat <[email protected]>
…here are deployments and no deployments. (#9) * Moved the utility method to nimUtils.ts
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Daniele Martinoli <[email protected]>
… model images then error message should be displayed. Added experimentalStudio flag to cypress.config.ts but disabled it by default.
… model images then error message should be displayed. Added experimentalStudio flag to cypress.config.ts but disabled it by default.
ff4c204
to
b1c2492
Compare
Signed-off-by: Tomer Figenblat <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3298 +/- ##
==========================================
+ Coverage 84.78% 85.49% +0.71%
==========================================
Files 1308 1309 +1
Lines 29247 29292 +45
Branches 7936 7955 +19
==========================================
+ Hits 24796 25043 +247
+ Misses 4451 4249 -202 see 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback. I'll finish up the review on Monday.
alphafold2: | ||
'{' + | ||
' "name": "alphafold2",' + | ||
' "displayName": "AlphaFold2",' + | ||
' "shortDescription": "A widely used model for predicting the 3D structures of proteins from their amino acid sequences.",' + | ||
' "namespace": "nim/deepmind",' + | ||
' "tags": [' + | ||
' "1.0.0"' + | ||
' ],' + | ||
' "latestTag": "1.0.0",' + | ||
' "updatedDate": "2024-08-27T01:51:55.642Z"' + | ||
' }', | ||
'arctic-embed-l': | ||
'{' + | ||
' "name": "arctic-embed-l",' + | ||
' "displayName": "Snowflake Arctic Embed Large Embedding",' + | ||
' "shortDescription": "NVIDIA NIM for GPU accelerated Snowflake Arctic Embed Large Embedding inference",' + | ||
' "namespace": "nim/snowflake",' + | ||
' "tags": [' + | ||
' "1.0.1",' + | ||
' "1.0.0"' + | ||
' ],' + | ||
' "latestTag": "1.0.1",' + | ||
' "updatedDate": "2024-07-27T00:38:40.927Z"' + | ||
' }', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps to make this easier to read just make it a normal JS object and do JSON.stringify
if you need a single string value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: baf470e
@@ -16,6 +16,7 @@ import { env, cypressEnv, BASE_URL } from '~/__tests__/cypress/cypress/utils/tes | |||
const resultsDir = `${env.CY_RESULTS_DIR || 'results'}/${env.CY_MOCK ? 'mocked' : 'e2e'}`; | |||
|
|||
export default defineConfig({ | |||
experimentalStudio: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this needed? Changing base configs I imagine is not necessary here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was helpful to debug by running the one testcase and add some html code by enabling this flag. it is disabled now if you want I am happy to take it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: baf470e
const project = mockProjectK8sResource({ | ||
hasAnnotations: true, | ||
enableModelMesh: hasAllModels ? undefined : false, | ||
}); | ||
if (project.metadata.annotations != null) { | ||
project.metadata.annotations['opendatahub.io/nim-support'] = 'true'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this belgons in the mocks for NIM Resource, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: baf470e
cy.intercept( | ||
{ method: 'GET', pathname: '/api/nim-serving/nvidia-nim-images-data' }, | ||
{ | ||
body: { body: mockNimImages() }, | ||
}, | ||
); | ||
cy.intercept( | ||
{ method: 'GET', pathname: '/api/nim-serving/nvidia-nim-access' }, | ||
{ body: { body: mockNvidiaNimAccessSecret() } }, | ||
); | ||
cy.intercept('GET', 'api/nim-serving/nvidia-nim-image-pull', { | ||
body: { body: mockNvidiaNimImagePullSecret() }, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to add to the existing list of interceptOdh
items found in frontend/src/__tests__/cypress/cypress/support/commands/odh.ts
It'll help with dealing with types & structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: 7da5544
cy.intercept('GET', '/api/accelerators', { | ||
configured: true, | ||
available: { 'nvidia.com/gpu': 1 }, | ||
total: { 'nvidia.com/gpu': 1 }, | ||
allocated: { 'nvidia.com/gpu': 1 }, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is interesting we don't have this one already. Would you mind adding this to our interceptOdh
list?
Also the type for the body is DetectedAccelerators
. We should always look to make sure objects are typed -- kinda surprised this was not flagged as a linter failure 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: 7da5544
|
||
export function validateNimModelsTable(): void { | ||
// Table is visible and has 2 rows (2nd is the hidden expandable row) | ||
cy.get('[data-testid="kserve-inference-service-table"]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely be using page objects and not being explicit about test ids like this. cy.findByTestId
for instance is also available.
Having page objects to reference items helps a lot when we need to mix tests in e2e with a cluster and the mocked need on PRs. Also helps with if additional tests need to be written.
ModelServingSection.getKServeTable
also does this select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: eba0293
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revisited here: aed9685
.should('have.length', 2); | ||
|
||
// First row matches the NIM inference service details | ||
cy.get('[style="display: block;"] > :nth-child(1)').should('have.text', 'Test Name'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unstable selector -- I would recommend that we don't want to do nth
access checks because they will always be brittle. What are you trying to select here -- again, page objects may help significantly here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: eaa0e47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments -- I tried to avoid making the same comment multiple times, but I may have missed the mark on that. General themes should be applied to all tests.
frontend/src/__tests__/cypress/cypress/tests/mocked/projects/modelServingNim.cy.ts
Outdated
Show resolved
Hide resolved
'Internal Service can be accessed inside the cluster', | ||
); | ||
// Opens the Models table | ||
cy.get('.pf-m-gap-md > :nth-child(2) > .pf-v5-c-button').click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels out of place. I'd not have expected validating the overview would navigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: 7c06f11
const overviewComponent = projectDetails.findComponent(componentName); | ||
overviewComponent.should('exist'); | ||
const deployModelButton = overviewComponent.findByTestId('model-serving-platform-button'); | ||
deployModelButton.should('exist'); | ||
validateNvidiaNimModel(deployModelButton); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an anti-pattern to store selected values -- make the full selector everytime
https://docs.cypress.io/guides/references/best-practices#Assigning-Return-Values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: cb01605
projectDetails | ||
.findComponent('overview') | ||
.findByTestId('single-serving-platform-card') | ||
.findByTestId('model-serving-platform-button') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be in page objects -- selecting like this raw in each test is not ideal for reuse or readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: cb01605
export function validateNimOverviewModelsTable(): void { | ||
// Card is visible | ||
cy.get( | ||
'.pf-v5-c-card__header-main > .pf-v5-l-flex > :nth-child(2) > .pf-v5-c-content > h3 > b', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid nth
checks & walking the DOM tree... these make brittle tests. If we need to, we should add test ids to components as necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: 7c06f11
cy.get('dt').should('have.text', 'Serving runtime'); | ||
cy.get('dd').should('have.text', 'NVIDIA NIM'); | ||
cy.get('[data-testid="internal-service-button"]').should('have.text', 'Internal Service'); | ||
cy.get('[data-testid="internal-service-button"]').click(); | ||
cy.get('.pf-v5-c-popover__title-text').should( | ||
'have.text', | ||
'Internal Service can be accessed inside the cluster', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page objects would help with this because it makes it easier to read and has declarative API for reuse.
Also cy.get
likely is rarely needed if we have proper test ids in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: 7c06f11
const deployModelButton = overviewComponent.findByTestId('model-serving-platform-button'); | ||
deployModelButton.should('exist'); | ||
deployModelButton.click(); | ||
cy.contains('There was a problem fetching the NIM models. Please try again later.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably scope checks for content to specific areas -- more specific text probably has less of a problem, but it seems a bit lacking in the page object department this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved here: cb01605
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
- Use JSON object instead of string in mocking of NIM images configmap - Remove experiementalStudio cypres config param - Add mock for NIM project Co-authored-by: Daniele Martinoli <[email protected]> Co-authored-by: lokeshrangineni <[email protected]> Signed-off-by: Tomer Figenblat <[email protected]>
- Add nim-serving interception to odh intercept type safety mechanism - Add accelerators interception to odh intercept type safety mechanism Co-authored-by: Daniele Martinoli <[email protected]> Co-authored-by: lokeshrangineni <[email protected]> Signed-off-by: Tomer Figenblat <[email protected]>
Co-authored-by: Daniele Martinoli <[email protected]> Co-authored-by: lokeshrangineni <[email protected]> Signed-off-by: Tomer Figenblat <[email protected]>
…odelServingNim.cy.ts Co-authored-by: Andrew Ballantyne <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Signed-off-by: Tomer Figenblat <[email protected]>
Description
Adding Cypress UI tests to cover the NIM Model use cases from all the supported pages and tabs:
How Has This Been Tested?
Running
npm run test:cypress-ci
command.Test Impact
Increases code coverage of UI code for all the components impacted by the NIM models.
Request review criteria:
Self checklist (all need to be checked):
After the PR is posted & before it merges:
main