Add E2E tests for model registry register-and-store OCI flow#6743
Add E2E tests for model registry register-and-store OCI flow#6743openshift-merge-bot[bot] merged 23 commits intoopendatahub-io:mainfrom
Conversation
openshift-ci
Bot
added
the
needs-ok-to-test
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds OCI-related metadata to model registry fixtures and the ModelRegistryTestData type, extends the Register Model page object with new FormFieldSelector entries and methods for namespace selection, registration-mode toggles, and origin (S3) / destination (OCI) sections, and introduces new end-to-end Cypress tests exercising a "register and store" flow targeting OCI. The tests include S3 origin credentials and OCI destination credentials and appear twice in the test file. objectStoragePathV2 value semantics remain unchanged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security-First ReviewCWE-798 — Use of Hard-coded Credentials — Actionable
CWE-219 / CWE-312 — Storage of Sensitive Data & Cleartext Transmission — Actionable
CWE-532 — Exposure of Sensitive Information in Logs — Actionable
CWE-200 — Information Exposure Through Error Messages — Actionable
Input validation & injection surface — Actionable
Actionable checklist (short):
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ 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. Comment |
|
Hi @Taj010. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/fixtures/e2e/modelRegistry/testModelRegistry.yaml`:
- Around line 81-86: The fixture contains credential-like literals
(ociSourceAccessKeyId, ociSourceSecretAccessKey, ociDestinationUsername,
ociDestinationPassword, ociDestinationUri) which must be removed; replace these
hard-coded secrets with non-sensitive placeholders (e.g., "<REDACTED>" or
"dummy-value") in the YAML and change tests to read real secrets from
environment/safe storage (Cypress.env or CI secrets) at runtime (e.g., use
Cypress.env('OCI_SOURCE_KEY') / Cypress.env('OCI_DEST_PASSWORD') or a
cypress.env.json that is gitignored), or document that the fixture is strictly
synthetic; ensure no real credentials are committed and update any code
referencing those keys (ociSourceAccessKeyId, ociSourceSecretAccessKey,
ociDestinationPassword) to gracefully handle missing env values and fail tests
with a clear message if required variables are not provided.
In `@packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts`:
- Around line 334-337: The test currently hard-asserts a transfer failure via
cy.contains(/Model transfer job failed/) which only occurs on PSI clusters; make
this environment-safe by branching on an environment flag or by checking for
either outcome: use Cypress.env (e.g., Cypress.env('isPSI') or
Cypress.env('transferFailsOnThisCluster')) to decide whether to assert the
failure via cy.contains(/Model transfer job failed/). Otherwise assert the
success case (e.g., cy.contains(/Model transfer (completed|succeeded)/) or the
existing success notification) so the test passes in non-PSI environments;
update the block using cy.step and cy.contains to perform this conditional
assertion.
- Around line 258-264: The test currently uses
registerModelPage.findFirstNamespaceOption() which selects whatever appears
first and causes ordering flakiness; change the selection to a deterministic
namespace lookup (e.g., use a known namespace string or a test fixture value) by
finding the option via its visible text or stable attribute instead of "first" —
locate where findFirstNamespaceOption() is used and replace it with a
deterministic selector call such as
registerModelPage.findNamespaceOptions().contains(<KNOWN_NAMESPACE>).click() (or
a new helper like
registerModelPage.findNamespaceOptionByName(<KNOWN_NAMESPACE>)) and capture that
known name into namespaceName for the cy.step. Ensure the chosen namespace is
stable across clusters and tests.
- Around line 301-320: The test is typing raw credentials from testData into
form fields (e.g., FormFieldSelector.LOCATION_S3_SECRET_ACCESS_KEY,
DESTINATION_OCI_PASSWORD) which risks leaking secrets in Cypress
logs/screenshots; change the test to read secrets from CI-provided environment
variables (Cypress.env('OCI_*') or a secure fixture) instead of hardcoded
testData and call .type(...) with logging disabled (pass { log: false }) for
those fields via registerModelPage.findFormField(...).type(..., { log: false })
so secrets are not emitted to Cypress command logs or artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ad1dfda0-4b07-41ff-9ed1-29dd70ee6c01
📥 Commits
Reviewing files that changed from the base of the PR and between 49d39ee and f167e5b8349e6d281dc3b026b43a6394285da90c.
📒 Files selected for processing (4)
packages/cypress/cypress/fixtures/e2e/modelRegistry/testModelRegistry.yamlpackages/cypress/cypress/pages/modelRegistry/registerModelPage.tspackages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.tspackages/cypress/cypress/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/registerModelPage.ts`:
- Around line 115-125: The helper findFirstNamespaceOption is dead code—remove
it—and make findNamespaceOption and findNamespaceListbox use scoped queries so
they only search within the namespace listbox context (avoid global role queries
that can hit other listboxes/options); update findNamespaceOption to locate the
namespace option by querying inside the element returned by findNamespaceListbox
and update any callers to use these scoped helpers instead of global role
queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cecc6a55-a6c7-4b16-92cb-33e92cdf7e4a
📥 Commits
Reviewing files that changed from the base of the PR and between 45b5f59f9990713fedbe3692725e68c8cadcf280 and 3dca549eea80d7ea478e133db9c3514a1fe37520.
📒 Files selected for processing (4)
packages/cypress/cypress/fixtures/e2e/modelRegistry/testModelRegistry.yamlpackages/cypress/cypress/pages/modelRegistry/registerModelPage.tspackages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.tspackages/cypress/cypress/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts
- packages/cypress/cypress/types.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6743 +/- ##
==========================================
- Coverage 64.81% 63.66% -1.15%
==========================================
Files 2441 2496 +55
Lines 75996 77538 +1542
Branches 19158 19703 +545
==========================================
+ Hits 49254 49363 +109
- Misses 26742 28175 +1433 see 62 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| it( | ||
| 'Exercises the register and store UI flow with OCI destination', | ||
| { | ||
| tags: ['@Dashboard', '@ModelRegistry', '@NonConcurrent'], |
There was a problem hiding this comment.
Would require a smoke/sanity tag for this test to run in pipeline.
Please add accordingly.
| .type(testData.ociSourceRegion); | ||
| registerModelPage.findFormField(FormFieldSelector.LOCATION_PATH).type(testData.ociSourcePath); | ||
|
|
||
| const sourceAccessKeyId = Cypress.env('OCI_SOURCE_ACCESS_KEY_ID') as string; |
There was a problem hiding this comment.
You could move all the constant initialization to top. See other tests for reference.
| registerModelPage.findSubmitButton().click(); | ||
|
|
||
| cy.step('Verify transfer job started notification appears'); | ||
| cy.contains(/Model transfer job started/, { timeout: 15000 }).should('be.visible'); |
There was a problem hiding this comment.
we are trying to avoid hardcoded string validation in tests. You could look for test-id or add this string in fixtures and use here for validation
| // and not validated here. A dedicated test with a controlled backend is more appropriate. | ||
| }, | ||
| ); | ||
|
|
||
| after(() => { |
There was a problem hiding this comment.
The after part is usually placed after the Before block. Please move it up. You could refer the other e2e tests.
|
Thanks @Taj010. Nice work! I added few comments. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts (1)
363-366:⚠️ Potential issue | 🟠 MajorMake the terminal-state assertion environment-aware.
Line 363 says this failure is PSI-specific, but the assertion still hard-requires failure on every cluster. That makes this smoke test cluster-dependent. Gate the expected terminal notification behind an env flag, or assert the matching success/failure fixture string per environment.
As per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts` around lines 363 - 366, The test currently always asserts the PSI-specific failure message using testData.ociTransferJobFailedNotification (inside the cy.step block and subsequent cy.contains call), which makes the spec cluster-dependent; update the assertion to be environment-aware by checking a Cypress env flag (e.g., Cypress.env('expectPSITransferFailure') or a cluster identifier) before asserting the failure message, and otherwise assert the success/alternate fixture string for non-PSI clusters (or skip the terminal-state assertion). Locate the block using cy.step('Verify transfer job failure notification...') and replace the unconditional cy.contains(testData.ociTransferJobFailedNotification...) with a conditional that selects the expected fixture based on the env flag or cluster value.
🤖 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/testRegisterModel.cy.ts`:
- Around line 73-94: The after() cleanup assumes setup completed and directly
dereferences variables like testData.uriModelName, registryName,
objectStorageModelName, and ociModelName; add a guard at the start of the
after() hook that returns early if setup didn’t finish (e.g., if testData or
registryName is falsy) and make each cleanup call conditional (only call
cleanupRegisteredModelsFromDatabase, deleteModelRegistry, checkModelRegistry,
deleteModelRegistryDatabase when their respective identifiers are defined) so
the after() hook will not throw secondary errors when loadModelRegistryFixture
or the before() hook fails.
- Around line 29-33: The four OCI environment reads (namespaceName,
sourceAccessKeyId, sourceSecretAccessKey, destinationUsername,
destinationPassword) are currently top-level and should be moved inside the
OCI-only test so missing creds fail fast; remove the top-level consts and
instead call Cypress.env(...) for each variable at the start of the OCI test
(the test that exercises the OCI registry/registration flow), assert each is
present (throw or use Cypress.fail) before any .type() or network actions, and
use those local variables for the steps that follow.
---
Duplicate comments:
In `@packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts`:
- Around line 363-366: The test currently always asserts the PSI-specific
failure message using testData.ociTransferJobFailedNotification (inside the
cy.step block and subsequent cy.contains call), which makes the spec
cluster-dependent; update the assertion to be environment-aware by checking a
Cypress env flag (e.g., Cypress.env('expectPSITransferFailure') or a cluster
identifier) before asserting the failure message, and otherwise assert the
success/alternate fixture string for non-PSI clusters (or skip the
terminal-state assertion). Locate the block using cy.step('Verify transfer job
failure notification...') and replace the unconditional
cy.contains(testData.ociTransferJobFailedNotification...) with a conditional
that selects the expected fixture based on the env flag or cluster value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 865868f9-30b4-402a-8ecb-ea85820b49df
📥 Commits
Reviewing files that changed from the base of the PR and between faecdc4ec9038c749cd825ce8f4c7c1e39996d03 and f9eb8bec41081c1c5038808f47cb1fc059ec642b.
📒 Files selected for processing (1)
packages/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts
ConorOM1
left a comment
ConorOM1
left a comment
There was a problem hiding this comment.
awesome work ! left a few minor comments
| cy.clearCookies(); | ||
| cy.clearLocalStorage(); | ||
| it( | ||
| 'Exercises the register and store UI flow with OCI destination', |
There was a problem hiding this comment.
i don't follow the wording here ? exercises ?
| .findFormField(FormFieldSelector.LOCATION_REGION) | ||
| .type(testData.ociSourceRegion); | ||
| registerModelPage.findFormField(FormFieldSelector.LOCATION_PATH).type(testData.ociSourcePath); | ||
|
|
There was a problem hiding this comment.
these variables can be removed, other tests pass Cypress.env() directly into .type(). Also, are the origin values not available in test-variables file already ?
| cy.step('Submit the register and store form'); | ||
| registerModelPage.findSubmitButton().click(); | ||
|
|
||
| cy.step('Verify transfer job started notification appears'); |
There was a problem hiding this comment.
after this step we could include a backend check for the relevant job/pod
| }, | ||
| () => { | ||
| cy.step('Log into the application'); | ||
| cy.visitWithLogin('/', HTPASSWD_CLUSTER_ADMIN_USER); |
There was a problem hiding this comment.
you can currently use '/?devFeatureFlags=true' here to test your script if needed
There was a problem hiding this comment.
actually it should be /?devFeatureFlags=registryOciStorage=true
| cy.step('Verify transfer job failure notification (expected on PSI due to no TLS)'); | ||
| cy.contains(testData.ociTransferJobFailedNotification, { timeout: 60000 }).should( | ||
| 'be.visible', | ||
| ); |
There was a problem hiding this comment.
could you also mention that TLS is not supported on a psi cluster environment
openshift-ci
Bot
added
area/automl
needs-rebase
|
/lgtm |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/ok-to-test |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
|
The Jenkins is tested in 209 |
|
/unhold |
|
/lgtm |
openshift-merge-bot
Bot
merged commit 89081d7
into
opendatahub-io:main
5 participants
Description
For RHOAIENG-49021
This PR adds E2E test coverage for the model registry "Register and store" feature along with page object helpers and fixture data to support it.
Specifically:
Adds a new test case (Exercises the register and store UI flow with OCI destination) that validates toggling to register-and-store mode, selecting a namespace, filling in origin S3 credentials, configuring OCI destination fields, and submitting the form.
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Tests