Task/APPENG-2738: NIM Cypress E2E tests #3298

dmartinol · 2024-10-04T10:03:24Z

Description

Adding Cypress UI tests to cover the NIM Model use cases from all the supported pages and tabs:

Show/Hide NVIDIA NIM model serving platform
Deploy NVIDIA NIM models
List NVIDIA NIM models
Delete NVIDIA NIM model

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):

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)

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

openshift-ci · 2024-10-04T10:03:36Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

lokeshrangineni · 2024-10-04T15:48:19Z

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 - applications/notebookServer.cy.ts

on local it is successful

andrewballantyne · 2024-10-04T19:17:20Z

/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]>

* 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: 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.

Signed-off-by: Tomer Figenblat <[email protected]>

codecov · 2024-10-04T20:46:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.49%. Comparing base (215d63d) to head (bd71dc4).
Report is 8 commits behind head on main.

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.

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

andrewballantyne

Some initial feedback. I'll finish up the review on Monday.

andrewballantyne · 2024-10-04T19:40:34Z

frontend/src/__mocks__/mockNimResource.ts

+      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"' +
+        '  }',
+    },


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.

Resolved here: baf470e

andrewballantyne · 2024-10-04T19:57:13Z

frontend/src/__tests__/cypress/cypress.config.ts

@@ -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,


Was this needed? Changing base configs I imagine is not necessary here...

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.

Resolved here: baf470e

andrewballantyne · 2024-10-04T20:00:39Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+  const project = mockProjectK8sResource({
+    hasAnnotations: true,
+    enableModelMesh: hasAllModels ? undefined : false,
+  });
+  if (project.metadata.annotations != null) {
+    project.metadata.annotations['opendatahub.io/nim-support'] = 'true';
+  }


I imagine this belgons in the mocks for NIM Resource, no?

Resolved here: baf470e

andrewballantyne · 2024-10-04T20:04:38Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+  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() },
+  });


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.

Resolved here: 7da5544

andrewballantyne · 2024-10-04T20:06:40Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+  cy.intercept('GET', '/api/accelerators', {
+    configured: true,
+    available: { 'nvidia.com/gpu': 1 },
+    total: { 'nvidia.com/gpu': 1 },
+    allocated: { 'nvidia.com/gpu': 1 },
+  });


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 🤔

Resolved here: 7da5544

andrewballantyne · 2024-10-04T20:10:03Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+
+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"]')


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.

Resolved here: eba0293

Revisited here: aed9685

andrewballantyne · 2024-10-04T20:27:23Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+    .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');


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?

Resolved here: eaa0e47

andrewballantyne

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

andrewballantyne · 2024-10-07T19:45:18Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+    '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();


This feels out of place. I'd not have expected validating the overview would navigate

Resolved here: 7c06f11

andrewballantyne · 2024-10-07T19:51:56Z

frontend/src/__tests__/cypress/cypress/tests/mocked/projects/modelServingNim.cy.ts

+        const overviewComponent = projectDetails.findComponent(componentName);
+        overviewComponent.should('exist');
+        const deployModelButton = overviewComponent.findByTestId('model-serving-platform-button');
+        deployModelButton.should('exist');
+        validateNvidiaNimModel(deployModelButton);


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

Resolved here: cb01605

andrewballantyne · 2024-10-07T19:53:21Z

frontend/src/__tests__/cypress/cypress/tests/mocked/projects/modelServingNim.cy.ts

+        projectDetails
+          .findComponent('overview')
+          .findByTestId('single-serving-platform-card')
+          .findByTestId('model-serving-platform-button')


These should be in page objects -- selecting like this raw in each test is not ideal for reuse or readability.

Resolved here: cb01605

andrewballantyne · 2024-10-07T19:55:27Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+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',


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.

Resolved here: 7c06f11

andrewballantyne · 2024-10-07T19:56:20Z

frontend/src/__tests__/cypress/cypress/utils/nimUtils.ts

+  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',
+  );


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.

Resolved here: 7c06f11

andrewballantyne · 2024-10-07T20:06:42Z

frontend/src/__tests__/cypress/cypress/tests/mocked/projects/modelServingNim.cy.ts

+        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.');


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

Resolved here: cb01605

openshift-ci · 2024-10-07T20:07:42Z

[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 andrewballantyne. For more information see the Kubernetes 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:

frontend/OWNERS

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

- 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]>

openshift-ci bot requested review from alexcreasy and christianvogt October 4, 2024 10:03

openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Oct 4, 2024

TomerFi force-pushed the nim-cypress-e2e-tests branch from 8480168 to e8be6e5 Compare October 4, 2024 14:51

openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Oct 4, 2024

TomerFi and others added 20 commits October 4, 2024 15:57

test: added mocks and interceptors for nim cypress tests

9947f96

Signed-off-by: Tomer Figenblat <[email protected]>

Merging the base test cases of Daniel and Lokesh's code to the Tomer'…

ba0f98c

…s standalone cypress test suite.

Reverting the mock changes which are not needed.

d103247

nim-specific classes and functions, partial rework of deployment test (…

b1e48d3

…#2) Signed-off-by: Daniele Martinoli <[email protected]>

Checking in the cypress test covers to validate all the scenarios to …

2c7152d

…render nim and all other cards on overview and model tab when there are no ServingRuntimes configured.

Added mockNimServingRuntimeTemplate mock function

2424ab3

Signed-off-by: Daniele Martinoli <[email protected]>

mocking deploy respurces (WIP)

8175aa6

Signed-off-by: Daniele Martinoli <[email protected]>

Completed deployment tests

1d80e39

Signed-off-by: Daniele Martinoli <[email protected]>

Added new test cases to validate if the nim is not enabled

b662f9f

* when combination of configurations to enable nim models are not configured properly.

test: added test cases for deleting model from the project models tab

47306a1

Signed-off-by: Tomer Figenblat <[email protected]>

* Added new test cases to validate the model serving menu item when t…

34ab9df

…here are deployments and no deployments. (#9) * Moved the utility method to nimUtils.ts

test: cleanups

e57074a

Signed-off-by: Tomer Figenblat <[email protected]>

adding const for modal dialog title

b5c58a8

Signed-off-by: Daniele Martinoli <[email protected]>

test cases for list of models in different pages/tabs

db769a1

Signed-off-by: Daniele Martinoli <[email protected]>

linting fixes

6d82381

Signed-off-by: Daniele Martinoli <[email protected]>

removed experimentalStudio option

5de4b14

Signed-off-by: Daniele Martinoli <[email protected]>

test: split nim tests per pages (#13)

a765cb9

Signed-off-by: Tomer Figenblat <[email protected]>

testing more buttons in modal dialog (#14)

e23d11a

Signed-off-by: Daniele Martinoli <[email protected]>

Added a test case to address there is a failure in loading Nvidia Nim…

ca6ef78

… model images then error message should be displayed. Added experimentalStudio flag to cypress.config.ts but disabled it by default.

Added a test case to address there is a failure in loading Nvidia Nim…

b1c2492

… model images then error message should be displayed. Added experimentalStudio flag to cypress.config.ts but disabled it by default.

TomerFi force-pushed the nim-cypress-e2e-tests branch from ff4c204 to b1c2492 Compare October 4, 2024 19:57

test: fix linting errors

c480d4e

Signed-off-by: Tomer Figenblat <[email protected]>

andrewballantyne requested changes Oct 4, 2024

View reviewed changes

openshift-ci bot assigned andrewballantyne Oct 4, 2024

lokeshrangineni mentioned this pull request Oct 4, 2024

incorporating code review fix. TomerFi/odh-dashboard#16

Merged

dmartinol mentioned this pull request Oct 7, 2024

Adding mockNimEnabledProject mocking function TomerFi/odh-dashboard#17

Closed

andrewballantyne requested changes Oct 7, 2024

View reviewed changes

TomerFi and others added 12 commits October 7, 2024 18:51

test: create page objects for verifying nim model tabels

eba0293

Co-authored-by: Daniele Martinoli <[email protected]> Co-authored-by: lokeshrangineni <[email protected]> Signed-off-by: Tomer Figenblat <[email protected]>

Update frontend/src/__tests__/cypress/cypress/tests/mocked/projects/m…

ec116f6

…odelServingNim.cy.ts Co-authored-by: Andrew Ballantyne <[email protected]>

test: fix linting errors

bdc58ab

Signed-off-by: Tomer Figenblat <[email protected]>

test: rewrite test case for global model list nim

eaa0e47

Signed-off-by: Tomer Figenblat <[email protected]>

test: fix linting errors

dd7ac09

Signed-off-by: Tomer Figenblat <[email protected]>

test: rewrite test case for project overview list nim

7c06f11

Signed-off-by: Tomer Figenblat <[email protected]>

test: rewrite test case for project models tab list nim

aed9685

Signed-off-by: Tomer Figenblat <[email protected]>

test: cleanups

1cafc5d

Signed-off-by: Tomer Figenblat <[email protected]>

test: rewrite test cases nim enablement

cb01605

Signed-off-by: Tomer Figenblat <[email protected]>

test: final touchups

bd71dc4

Signed-off-by: Tomer Figenblat <[email protected]>

Task/APPENG-2738: NIM Cypress E2E tests #3298

Are you sure you want to change the base?

Task/APPENG-2738: NIM Cypress E2E tests #3298

Conversation

dmartinol commented Oct 4, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 4, 2024

lokeshrangineni commented Oct 4, 2024 • edited Loading

andrewballantyne commented Oct 4, 2024

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

andrewballantyne left a 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

TomerFi Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a 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

openshift-ci bot commented Oct 7, 2024

lokeshrangineni commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

TomerFi Oct 8, 2024 •

edited

Loading