Skip to content

[Cypress-e2e] Add model registry custom properties retention test#7313

Merged
openshift-merge-bot[bot] merged 7 commits intoopendatahub-io:mainfrom
ConorOM1:RHOAIENG-20681
Apr 24, 2026
Merged

[Cypress-e2e] Add model registry custom properties retention test#7313
openshift-merge-bot[bot] merged 7 commits intoopendatahub-io:mainfrom
ConorOM1:RHOAIENG-20681

Conversation

@ConorOM1
Copy link
Copy Markdown
Contributor

@ConorOM1 ConorOM1 commented Apr 20, 2026

https://redhat.atlassian.net/browse/RHOAIENG-20681

Description

Adds an E2E test verifying that custom properties on a registered model and its version are not lost during model registry operations.

Includes new page objects for registered model and version details pages, DB check utilities for custom properties, and supporting fixture and type definitions.

How Has This Been Tested?

Jenkins: job/dashboard-e2e-tests/287/

Test Impact

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)
  • The code follows our Best Practices (React coding standards, PatternFly usage, performance considerations)

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

Summary by CodeRabbit

  • Tests
    • Added end-to-end test coverage for custom properties retention in the Model Registry.
    • Includes comprehensive validation of model and version-level custom properties persistence across UI and backend storage.
    • Introduces test fixtures and utilities to support quality assurance of custom properties functionality.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 20, 2026

Warning

Rate limit exceeded

@ConorOM1 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 59 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 59 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 434e597b-0ba9-4348-ae34-f1abbe0e9ad4

📥 Commits

Reviewing files that changed from the base of the PR and between 0e2391a and cc5ccac.

📒 Files selected for processing (1)
  • packages/cypress/cypress/types.ts
📝 Walkthrough

Walkthrough

This pull request adds comprehensive testing infrastructure for custom properties retention in the Model Registry. It introduces a new Cypress E2E test that verifies custom properties can be added at both model and version levels, persist correctly in the backend SQL database, and remain intact across UI navigation. Supporting changes include a YAML fixture providing test data, new page object methods and a new page object class for interacting with properties UI elements, type definitions for test configuration, and a database query utility that verifies property persistence via SQL.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Security & Code Quality Findings

SQL Injection Risk in checkCustomPropertiesInDatabase
The utility constructs a SQL LIKE clause with unsanitized user input:

c.name LIKE '%${entityName}%'

Even though this executes in a test context via oc exec, it bypasses parameter binding. CWE-89. Use parameterized queries or escape the input with MySQL escaping functions.

Incomplete Error Handling in Database Utility
The function logs and returns false when a MySQL pod is not found, but fails silently on non-zero query exit without distinguishing between connection failures and query logic errors. This masks debugging of legitimate issues during test execution.

Test Data Validation Gap
The fixture YAML provides custom properties as simple key-value pairs with no validation that keys conform to whatever naming/format constraints the actual Model Registry enforces. Tests may pass with fixture data that real API calls would reject.

Missing Async State Verification
The E2E test performs database checks immediately after UI operations without explicit wait/retry logic for replication lag, assuming synchronous persistence. This introduces flakiness if the backend has async writes.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly describes the main change: adding an E2E test for model registry custom properties retention.
Description check ✅ Passed Description references the Jira issue, explains what is being tested and what components were added, and provides Jenkins test evidence. However, the self-checklist is incomplete and test impact section is empty.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (2)
packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts (1)

11-12: Do not target version properties by DOM order.

.last() makes this page object depend on current layout order, so adding another properties section can make the test edit/assert the wrong object. Add a stable wrapper data-testid for the version details card and scope from there.

Scoped selector sketch
+  findVersionDetailsCard() {
+    return cy.findByTestId('model-version-details-card');
+  }
+
   findPropertiesExpandableSection() {
-    return cy.findAllByTestId('properties-expandable-section').last();
+    return this.findVersionDetailsCard().findByTestId('properties-expandable-section');
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts` around
lines 11 - 12, The selector findPropertiesExpandableSection() currently relies
on DOM order via .last(), which is brittle; update it to scope the search to a
stable version-details wrapper: locate the version details card (e.g.,
data-testid "version-details-card" or similar) and then query for the properties
section inside it (e.g., use
cy.findByTestId('version-details-card').findByTestId('properties-expandable-section')
or cy.get('[data-testid="version-details-card"]').within(() =>
cy.findByTestId('properties-expandable-section'))), removing the .last() usage
so the test targets the correct version's properties reliably.
packages/cypress/cypress/types.ts (1)

534-544: Split this into a fixture-specific type.

These required fields are specific to testRetainCustomProperties.yaml, while that fixture still omits many existing required ModelRegistryTestData fields and other model-registry fixtures omit these new fields. The shared type no longer represents a real fixture shape. Add a dedicated retention-test type and use it in the new spec.

Type split sketch
+export type ModelRegistryCustomProperty = {
+  key: string;
+  value: string;
+};
+
+export type ModelRegistryCustomPropertiesRetentionTestData = {
+  registryNamePrefix: string;
+  databaseNamePrefix: string;
+  projectNamePrefix: string;
+  operatorDeploymentName: string;
+  modelNamePrefix: string;
+  modelDescription: string;
+  modelCustomProperties: ModelRegistryCustomProperty[];
+  modelLabels: string[];
+  versionName: string;
+  versionDescription: string;
+  versionCustomProperties: ModelRegistryCustomProperty[];
+  versionLabels: string[];
+  sourceModelFormat: string;
+  sourceModelFormatVersion: string;
+  updatedVersionDescription: string;
+  updatedModelFormat: string;
+  updatedFormatVersion: string;
+  newVersionLabel: string;
+  newVersionPropertyKey: string;
+  newVersionPropertyValue: string;
+  objectStorageEndpoint: string;
+  objectStorageBucket: string;
+  objectStorageRegion: string;
+  objectStoragePath: string;
+  modelFormatForDeployment: string;
+  servingRuntimeForDeployment: string;
+  modelPathForDeployment: string;
+};
+
 export type ModelRegistryTestData = {
   registryNamePrefix: string;
   createRegistryName: string;
@@
-  // Custom properties retention test configuration
-  modelNamePrefix: string;
-  modelDescription: string;
-  versionName: string;
-  versionDescription: string;
-  sourceModelFormat: string;
-  sourceModelFormatVersion: string;
-  modelCustomProperties: Array<{ key: string; value: string }>;
-  versionCustomProperties: Array<{ key: string; value: string }>;
-  newVersionPropertyKey: string;
-  newVersionPropertyValue: string;
 };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cypress/cypress/types.ts` around lines 534 - 544, The current shared
fixture type includes fields specific to the retention test (modelNamePrefix,
modelDescription, versionName, versionDescription, sourceModelFormat,
sourceModelFormatVersion, modelCustomProperties, versionCustomProperties,
newVersionPropertyKey, newVersionPropertyValue), which breaks other fixtures;
create a new fixture-specific interface (e.g., RetainCustomPropertiesFixture or
ModelRegistryRetentionTestData) containing only those retention-specific fields,
remove them from the shared ModelRegistryTestData, and update the retention spec
to import/use the new fixture type (adjust any references to modelNamePrefix,
modelCustomProperties, etc. to the new type) so other fixtures keep the original
shared shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts`:
- Around line 58-65: The addCustomProperty helper is non-deterministic because
it blindly clicks the whole expandable section (findPropertiesExpandableSection)
which may toggle it closed; change it to call ensurePropertiesExpanded() first,
then scope subsequent queries to that expanded section element (use the section
element returned by ensurePropertiesExpanded or re-query a scoped container)
before invoking findAddPropertyButton(), findPropertyKeyInput(),
findPropertyValueInput(), and findSavePropertyButton() so the clicks/types
operate on the visible controls reliably.
- Around line 48-51: The test in findPropertiesExpandableSection() is asserting
a PatternFly implementation class ('.pf-v6-c-expandable-section__content') and
css visibility; replace this with a stable selector and assertion: target a
data-testid on the expandable content (e.g.
[data-testid="properties-expandable-content"]) or assert the toggle's state via
its ARIA attribute (e.g. the toggle element's aria-expanded) instead of the PF
class; update the call in registeredModelDetails.ts to locate the content by
data-testid or check the toggle element's aria-expanded value and assert
visibility/assertion accordingly so the test no longer relies on PatternFly CSS
classes.

In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts`:
- Around line 29-34: The test's teardown assumes variables (modelName,
registryName, deploymentName, testData) are always set and can throw if before()
failed partially; make those variables nullable (e.g., string | undefined for
modelName, registryName, deploymentName and ModelRegistryTestData | undefined
for testData) and update the after() cleanup to only attempt deletion when the
corresponding identifier is defined (guard accesses in the cleanup helpers
called from after() and check registryName/modelName/deploymentName before
calling any delete/unregister functions). Ensure the same guards exist wherever
the fixture-load callback previously populated these symbols so cleanup is
skipped for resources that were never created.

In `@packages/cypress/cypress/utils/oc_commands/modelRegistry.ts`:
- Around line 1051-1052: The test only verifies keys; change the DB query in the
helper to select cp.name and the schema’s string-value column (e.g., value) so
the result returns [{name, value}], then replace the foundKeys/hasAllKeys logic
(currently using output.split(...), foundKeys, expectedPropertyKeys) to parse
and compare {key, value} pairs against the fixture’s property objects (pass the
fixture property arrays instead of key arrays at the call sites around the other
helper usages). Update the helper that uses foundKeys/hasAllKeys to build
foundProperties = output.rows.map(r => ({ key: r.name, value: r.value })) and
assert every expected property object is present and equal to a matching found
property.
- Around line 1007-1009: Remove the failOnNonZeroExit override in the cy.exec
call (the pod selector -l name=${databaseName} is correct), and replace the
hardcoded password string used when building verifyCommand with a Cypress
environment variable (use Cypress.env('DATABASE_PASSWORD') assigned to a local
password variable); when constructing verifyCommand, interpolate that password
(not the literal -pTheBlurstOfTimes), ensure the mysql invocation includes
--skip-column-names as shown, and defensively quote the ${podName} substitution
in the oc exec invocation (reference variables/identifiers: targetNamespace,
databaseName, podName, verifyCommand, password).
- Around line 1020-1022: Replace the hardcoded password and unquoted shell
inputs by reading the DB password from Cypress.env (e.g.,
Cypress.env('MODEL_REGISTRY_PASSWORD')), inject it into the environment as
MYSQL_PWD (not via -p) when calling oc/mysql, and properly shell-quote dynamic
values; create/consume a small helper like shellEscape(value) that escapes
single quotes and returns a single-quoted string, then use it to wrap podName,
targetNamespace and escapedModelName in the oc exec command (replace occurrences
where verifyCommand is built and the 4 other similar places). Also pass the env
option to Cypress.exec (or equivalent) with MYSQL_PWD set and include log: false
so credentials are not printed; update any callsites that build verifyCommand
(variables sqlQuery, verifyCommand and related functions) to use these changes
across all five locations (lines referenced) so no plaintext password or
unquoted shell inputs remain.

---

Nitpick comments:
In `@packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts`:
- Around line 11-12: The selector findPropertiesExpandableSection() currently
relies on DOM order via .last(), which is brittle; update it to scope the search
to a stable version-details wrapper: locate the version details card (e.g.,
data-testid "version-details-card" or similar) and then query for the properties
section inside it (e.g., use
cy.findByTestId('version-details-card').findByTestId('properties-expandable-section')
or cy.get('[data-testid="version-details-card"]').within(() =>
cy.findByTestId('properties-expandable-section'))), removing the .last() usage
so the test targets the correct version's properties reliably.

In `@packages/cypress/cypress/types.ts`:
- Around line 534-544: The current shared fixture type includes fields specific
to the retention test (modelNamePrefix, modelDescription, versionName,
versionDescription, sourceModelFormat, sourceModelFormatVersion,
modelCustomProperties, versionCustomProperties, newVersionPropertyKey,
newVersionPropertyValue), which breaks other fixtures; create a new
fixture-specific interface (e.g., RetainCustomPropertiesFixture or
ModelRegistryRetentionTestData) containing only those retention-specific fields,
remove them from the shared ModelRegistryTestData, and update the retention spec
to import/use the new fixture type (adjust any references to modelNamePrefix,
modelCustomProperties, etc. to the new type) so other fixtures keep the original
shared shape.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: f67e25a3-3cbd-4ba9-bed2-0b2ced5a7619

📥 Commits

Reviewing files that changed from the base of the PR and between 46ad36a and 4d0ca56.

📒 Files selected for processing (7)
  • packages/cypress/cypress/fixtures/e2e/modelRegistry/testRetainCustomProperties.yaml
  • packages/cypress/cypress/pages/modelRegistry.ts
  • packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts
  • packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts
  • packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts
  • packages/cypress/cypress/types.ts
  • packages/cypress/cypress/utils/oc_commands/modelRegistry.ts

Comment thread packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts Outdated
Comment thread packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts Outdated
Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts Outdated
Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts Outdated
Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.91%. Comparing base (85c40b3) to head (cc5ccac).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7313      +/-   ##
==========================================
- Coverage   65.15%   63.91%   -1.24%     
==========================================
  Files        2458     2513      +55     
  Lines       76457    77994    +1537     
  Branches    19300    19840     +540     
==========================================
+ Hits        49812    49849      +37     
- Misses      26645    28145    +1500     
Files with missing lines Coverage Δ
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx 1.85% <ø> (ø)

... and 75 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 85c40b3...cc5ccac. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sridarna
Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added lgtm and removed lgtm labels Apr 21, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/cypress/cypress/utils/oc_commands/modelRegistry.ts (1)

1000-1143: DRY: checkModelCustomPropertiesInDatabase and checkVersionCustomPropertiesInDatabase are ~95% identical.

The two helpers differ only in the log wording and the WHERE clause (= vs LIKE '%...%'). Pod discovery, error handling, output parsing, and the hasAllKeys comparison are copy-pasted — any future fix (e.g., value comparison, shell-quoting, LIKE escaping) has to be applied in two places and the past review history already shows one such fix landing asymmetrically.

Extract a private helper that takes a WHERE predicate (or { contextName, matchType: 'exact' | 'like' }) and have both exports delegate to it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cypress/cypress/utils/oc_commands/modelRegistry.ts` around lines
1000 - 1143, The two functions checkModelCustomPropertiesInDatabase and
checkVersionCustomPropertiesInDatabase are nearly identical; extract a private
helper (e.g., checkCustomPropertiesInDatabase or
_checkCustomPropertiesInDatabase) that accepts { contextName, matchType: 'exact'
| 'like', databaseName, expectedPropertyKeys } and encapsulates pod discovery,
SQL construction (use '=' for 'exact' and "LIKE '%...%'" for 'like' with proper
single-quote escaping via replace(/'/g,"''")), the oc exec call, output parsing,
hasAllKeys comparison and logging; then refactor
checkModelCustomPropertiesInDatabase to call the helper with matchType='exact'
and checkVersionCustomPropertiesInDatabase to call it with matchType='like',
removing duplicated logic from both original functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/cypress/cypress/utils/oc_commands/modelRegistry.ts`:
- Around line 1096-1097: The SQL uses versionName interpolated into a LIKE
'%...%' pattern but only single quotes are escaped (escapedVersionName), so '%'
'_' and '\' in versionName can create unintended wildcard matches; update the
construction in modelRegistry.ts to escape LIKE metacharacters in versionName
(replace % with \%, _ with \_, and \ with \\) before embedding, then use that
escaped value in sqlQuery (the variable names to modify are versionName ->
escapedVersionName and sqlQuery) and include an explicit ESCAPE clause in the
query (or switch to an exact-match pattern built from the
registeredModelId:versionName naming convention) so the query matches the
intended context name instead of allowing wildcard collisions.

---

Nitpick comments:
In `@packages/cypress/cypress/utils/oc_commands/modelRegistry.ts`:
- Around line 1000-1143: The two functions checkModelCustomPropertiesInDatabase
and checkVersionCustomPropertiesInDatabase are nearly identical; extract a
private helper (e.g., checkCustomPropertiesInDatabase or
_checkCustomPropertiesInDatabase) that accepts { contextName, matchType: 'exact'
| 'like', databaseName, expectedPropertyKeys } and encapsulates pod discovery,
SQL construction (use '=' for 'exact' and "LIKE '%...%'" for 'like' with proper
single-quote escaping via replace(/'/g,"''")), the oc exec call, output parsing,
hasAllKeys comparison and logging; then refactor
checkModelCustomPropertiesInDatabase to call the helper with matchType='exact'
and checkVersionCustomPropertiesInDatabase to call it with matchType='like',
removing duplicated logic from both original functions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f7291b3-bd8e-4477-848e-a20ed5278cc5

📥 Commits

Reviewing files that changed from the base of the PR and between 4d0ca56 and 24a643a.

📒 Files selected for processing (7)
  • packages/cypress/cypress/fixtures/e2e/modelRegistry/testRetainCustomProperties.yaml
  • packages/cypress/cypress/pages/modelRegistry.ts
  • packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts
  • packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts
  • packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts
  • packages/cypress/cypress/types.ts
  • packages/cypress/cypress/utils/oc_commands/modelRegistry.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/cypress/cypress/fixtures/e2e/modelRegistry/testRetainCustomProperties.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts
  • packages/cypress/cypress/types.ts
  • packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts
  • packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts

Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts Outdated
Comment thread packages/cypress/cypress/pages/modelRegistry/modelVersionDetails.ts Outdated
Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts Outdated
Comment thread packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts Outdated
}

findPropertiesToggleButton() {
return this.findPropertiesExpandableSection().find('button[aria-expanded]').first();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a data-testid here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unfortunately looks like PF does not support passing props to the internal toggle button

@ConorOM1
Copy link
Copy Markdown
Contributor Author

tested locally due to addition of new test-ids:

image

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (2)
packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts (1)

173-180: Weaker save-confirmation than the model-side loop.

On the model side (Line 147) you assert findPropertyKeyInput().should('not.exist') after save, which proves the editor closed and the save actually committed. Here you only check the expandable section is visible, which can be true regardless of whether the save round-tripped. This will let a silent save failure leak into the subsequent DB assertion and confuse the failure signal. Mirror the model-side check.

🔧 Proposed fix
       testData.versionCustomProperties.forEach((prop) => {
         modelVersionDetails.findAddPropertyButton().scrollIntoView().click();
         modelVersionDetails.findPropertyKeyInput().type(prop.key);
         modelVersionDetails.findPropertyValueInput().type(prop.value);
         modelVersionDetails.findSavePropertyButton().click();
-        modelVersionDetails.findPropertiesExpandableSection().should('be.visible');
+        modelVersionDetails.findPropertyKeyInput().should('not.exist');
       });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts`
around lines 173 - 180, The save confirmation after adding each custom property
is too weak: instead of only asserting
modelVersionDetails.findPropertiesExpandableSection().should('be.visible'),
after clicking modelVersionDetails.findSavePropertyButton() assert that the
editor closed by checking
modelVersionDetails.findPropertyKeyInput().should('not.exist') (mirroring the
model-side loop), so the test guarantees the save round-tripped before
proceeding to DB assertions.
packages/cypress/cypress/types.ts (1)

534-544: Type is out of sync with fixture — several fields in testRetainCustomProperties.yaml are not modeled here.

The fixture declares databaseNamePrefix, projectNamePrefix, modelLabels, versionLabels, updatedVersionDescription, updatedModelFormat, updatedFormatVersion, newVersionLabel, modelFormatForDeployment, servingRuntimeForDeployment, modelPathForDeployment. None are on ModelRegistryTestData. Either drop them from the YAML (dead fixture data) or add them to the type so future tests get type-checking and the fixture cannot drift silently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cypress/cypress/types.ts` around lines 534 - 544, The type
ModelRegistryTestData is missing fields present in the test fixture
(testRetainCustomProperties.yaml), causing the fixture and type to drift; update
the ModelRegistryTestData interface (or type) to include the missing properties:
databaseNamePrefix, projectNamePrefix, modelLabels, versionLabels,
updatedVersionDescription, updatedModelFormat, updatedFormatVersion,
newVersionLabel, modelFormatForDeployment, servingRuntimeForDeployment, and
modelPathForDeployment with appropriate types (strings or arrays of strings as
used by the fixture) so the fixture is fully typed and future changes are caught
by the compiler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts`:
- Around line 188-193: The test uses a static testData.versionName ("v1.0")
which makes the checkCustomPropertiesInDatabase(..., 'like') query fragile;
update the test setup to suffix testData.versionName with the same uuid you
already add to registryName/modelName (e.g., testData.versionName =
`${testData.versionName}-${uuid}`) so all uses of testData.versionName
(including the checkCustomPropertiesInDatabase calls around the version checks
and the other occurrence later) are unique to this test run and the LIKE
predicate can only match this run's version; adjust any assertions or subsequent
uses to reference the newly suffixed testData.versionName.
- Line 29: The describe title claims both custom properties and labels are
retained but the spec never uses testData.modelLabels or testData.versionLabels;
either add UI and DB steps to create and assert labels (referencing
testData.modelLabels/testData.versionLabels and the existing test cases that
handle properties) so the spec exercises labels end-to-end, or simplify by
removing "and labels" from the describe string and deleting the unused
testData.modelLabels/testData.versionLabels entries from the fixture to avoid
misleading scope.

In `@packages/cypress/cypress/utils/oc_commands/modelRegistry.ts`:
- Around line 1008-1025: The pod-name parsing can yield multiple pods and break
the oc exec command; update the logic that derives podName from podResult (the
podResult -> podName assignment used by verifyCommand) to select only the first
line before stripping the "pod/" prefix (e.g., split on newline and take [0] or
pipe the oc get command through "head -1"), mirroring the approach used in
checkModelExistsInDatabase / checkModelVersionExistsInDatabase /
cleanupRegisteredModelsFromDatabase so oc exec always receives a single valid
pod name.

---

Nitpick comments:
In
`@packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts`:
- Around line 173-180: The save confirmation after adding each custom property
is too weak: instead of only asserting
modelVersionDetails.findPropertiesExpandableSection().should('be.visible'),
after clicking modelVersionDetails.findSavePropertyButton() assert that the
editor closed by checking
modelVersionDetails.findPropertyKeyInput().should('not.exist') (mirroring the
model-side loop), so the test guarantees the save round-tripped before
proceeding to DB assertions.

In `@packages/cypress/cypress/types.ts`:
- Around line 534-544: The type ModelRegistryTestData is missing fields present
in the test fixture (testRetainCustomProperties.yaml), causing the fixture and
type to drift; update the ModelRegistryTestData interface (or type) to include
the missing properties: databaseNamePrefix, projectNamePrefix, modelLabels,
versionLabels, updatedVersionDescription, updatedModelFormat,
updatedFormatVersion, newVersionLabel, modelFormatForDeployment,
servingRuntimeForDeployment, and modelPathForDeployment with appropriate types
(strings or arrays of strings as used by the fixture) so the fixture is fully
typed and future changes are caught by the compiler.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: fc6efc77-d622-4ffc-8774-91041d55e8e7

📥 Commits

Reviewing files that changed from the base of the PR and between 24a643a and 0e2391a.

⛔ Files ignored due to path filters (1)
  • packages/model-registry/upstream/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx is excluded by !**/upstream/**
📒 Files selected for processing (4)
  • packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts
  • packages/cypress/cypress/tests/e2e/modelRegistry/testRetainCustomProperties.cy.ts
  • packages/cypress/cypress/types.ts
  • packages/cypress/cypress/utils/oc_commands/modelRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cypress/cypress/pages/modelRegistry/registeredModelDetails.ts

Comment thread packages/cypress/cypress/utils/oc_commands/modelRegistry.ts
@ConorOM1 ConorOM1 requested a review from FedeAlonso April 24, 2026 13:14
@FedeAlonso
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 24, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeAlonso

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

The pull request process is described here

Details 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

@openshift-merge-bot openshift-merge-bot Bot merged commit bb045de into opendatahub-io:main Apr 24, 2026
52 checks passed
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.

3 participants