Add Model-registry cypress and unit tests to the CI#7338
Add Model-registry cypress and unit tests to the CI#7338ppadti wants to merge 3 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: ppadti <ppadti@redhat.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Cypress page-object modules for Model Details (card and expanded view) and Model Version Details, exposing element finders and helpers for description, labels, properties, and kebab actions. Adds new Cypress tests: MCP catalog deploy-button visibility cases (404 and empty-artifacts scenarios) and a comprehensive model-registry details-editing suite with mocked intercepts, PATCH assertions, label/description/property edit flows, and client-side validation tests for label length and duplicates. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security FindingsNo actionable security issues identified. Changes are test/page-object code using Cypress stubs and selectors; no new production logic or sensitive-data handling introduced. No relevant CWE/CVE entries applicable. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts (1)
176-192: Monolithic test conflates six independent scenarios.
should show alerts for the expanded sectiondrives label edit → save → description edit → save → property edit → save → add-property flows in oneit. If any earlier assertion fails, the remaining alert behaviors are never exercised, and the failure message won't tell you which flow is actually broken. Split into per-flowitblocks (label / description / property-edit / add-property) so failures are localized and the suite stays diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts` around lines 176 - 192, The test "should show alerts for the expanded section" is doing six independent checks in one it block; split it into separate it blocks each asserting alert behavior for one flow (label edit/save, description edit/save, property edit/save, add-property) so failures are isolated. Locate the existing test using the it name and refactor into four tests that reuse modelDetailsExpandedCard setup (click findExpandedButton and assert visibility), then in each new it call only the relevant actions and assertion: use findLabelEditButton/findLabelSaveButton for the label test, findDescriptionEditButton/findDescriptionSaveButton for the description test, getRow('property1').findKebabAction('Edit') and propertyRow.findSaveButton for property-edit test, and findAddPropertyButton for add-property test, each followed by modelDetailsExpandedCard.findAlert().should('exist').
🤖 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/mocked/mcpCatalog/mcpDeployments.cy.ts`:
- Line 434: Replace the brittle text-based assertion cy.contains('MCP server not
found').should('be.visible') with the new page-object finder that targets the
not-found element by its data-testid; update the test to import and use the
finder from packages/cypress/cypress/pages/mcpDeployments.ts (the finder name
e.g., mcpDeployments.getNotFound or similar) and assert visibility via that
finder instead of cy.contains so the test relies on the stable data-testid
selector.
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`:
- Line 275: The regex passed to cy.contains (used in the assertions that check
"Testing label already exists" and "can't exceed 63 characters") incorrectly
includes the global /g flag which makes the RegExp stateful and causes flaky
retries; remove the /g flag (use a plain RegExp like /Testing label already
exists|can't exceed 63 characters/ or two separate assertions/strings) for both
occurrences so cy.contains receives a non-global regex and retries behave
deterministically.
- Line 181: The test is clicking modelDetailsExpandedCard.findLabelSaveButton()
immediately after findLabelEditButton() without making any change, which can be
a no-op; update the test so after invoking
modelDetailsExpandedCard.findLabelEditButton().click() you either type a change
into the label input (e.g., append or set some text) before calling
modelDetailsExpandedCard.findLabelSaveButton().click() or explicitly exit edit
mode via the component’s cancel/close affordance instead of saving; apply the
same fix for propertyRow.findSaveButton() — ensure you reference the label input
or cancel control used by findLabelEditButton/findLabelSaveButton and
propertyRow.findSaveButton so the subsequent findDescriptionEditButton().click()
is not brittle.
---
Nitpick comments:
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`:
- Around line 176-192: The test "should show alerts for the expanded section" is
doing six independent checks in one it block; split it into separate it blocks
each asserting alert behavior for one flow (label edit/save, description
edit/save, property edit/save, add-property) so failures are isolated. Locate
the existing test using the it name and refactor into four tests that reuse
modelDetailsExpandedCard setup (click findExpandedButton and assert visibility),
then in each new it call only the relevant actions and assertion: use
findLabelEditButton/findLabelSaveButton for the label test,
findDescriptionEditButton/findDescriptionSaveButton for the description test,
getRow('property1').findKebabAction('Edit') and propertyRow.findSaveButton for
property-edit test, and findAddPropertyButton for add-property test, each
followed by modelDetailsExpandedCard.findAlert().should('exist').
🪄 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: d8c4f632-a30e-4f20-9057-20e9f50fc11c
⛔ Files ignored due to path filters (33)
.github/workflows/model-registry-frontend-tests.ymlis excluded by!.github/**packages/model-registry/upstream/frontend/config/moduleFederation.jsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/config/webpack.prod.jsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/package-lock.jsonis excluded by!**/package-lock.json,!**/upstream/**packages/model-registry/upstream/frontend/package.jsonis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/appChrome.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelCatalog.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistry.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/modelTransferJobs.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/modelVersionArchive.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/modelVersionDetails.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/registerAndStoreFields.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/registerModelPage.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/registerVersionPage.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/registeredModelArchive.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/support/e2e.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/mcpCatalog/mcpServerDetails.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalogTabs.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/tensorTypeComparisonCard.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalogSettings/modelCatalogSettings.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelDetailsCard.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelTransferJobs.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionArchive.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersions.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionsCard.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerAndStoreFields.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/utils/modelCatalogTestRoutes.tsis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/bootstrap.tsxis excluded by!**/upstream/**packages/model-registry/upstream/frontend/src/odh/components/MRDeploymentsContextProvider.tsxis excluded by!**/upstream/**
📒 Files selected for processing (4)
packages/cypress/cypress/pages/modelRegistry/modelDetailsCard.tspackages/cypress/cypress/pages/modelRegistry/modelVersionDetails.tspackages/cypress/cypress/tests/mocked/mcpCatalog/mcpDeployments.cy.tspackages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts
|
|
||
| cy.visitWithLogin('/ai-hub/mcp-servers/catalog/invalid-server'); | ||
|
|
||
| cy.contains('MCP server not found').should('be.visible'); |
There was a problem hiding this comment.
Use a data-testid-backed finder for the not-found state.
cy.contains() makes this test depend on copy text. Add a page-object finder for the not-found state and assert through that instead.
Proposed test-side change
- cy.contains('MCP server not found').should('be.visible');
+ mcpServerDetailsPage.findNotFoundState().should('contain.text', 'MCP server not found');Add the corresponding finder in packages/cypress/cypress/pages/mcpDeployments.ts after adding a stable data-testid to the not-found UI:
class McpServerDetailsPage {
+ findNotFoundState(): Cypress.Chainable<JQuery<HTMLElement>> {
+ return cy.findByTestId('mcp-server-not-found');
+ }
+
findDeployButton(): Cypress.Chainable<JQuery<HTMLElement>> {
return cy.findByTestId('mcp-deploy-button');
}As per coding guidelines, packages/cypress/**/*.{ts,js} tests should “Use data-testid selectors, not CSS classes (resilient to style changes).”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/tests/mocked/mcpCatalog/mcpDeployments.cy.ts` at
line 434, Replace the brittle text-based assertion cy.contains('MCP server not
found').should('be.visible') with the new page-object finder that targets the
not-found element by its data-testid; update the test to import and use the
finder from packages/cypress/cypress/pages/mcpDeployments.ts (the finder name
e.g., mcpDeployments.getNotFound or similar) and assert visibility via that
finder instead of cy.contains so the test relies on the stable data-testid
selector.
| modelDetailsExpandedCard.find().should('be.visible'); | ||
| modelDetailsExpandedCard.findLabelEditButton().click(); | ||
| modelDetailsExpandedCard.findAlert().should('exist'); | ||
| modelDetailsExpandedCard.findLabelSaveButton().click(); |
There was a problem hiding this comment.
Clicking findLabelSaveButton without any label change is likely a no-op or a silent failure.
Nothing was typed/added between findLabelEditButton().click() (line 179) and findLabelSaveButton().click() here. Depending on the component, the save button may be disabled (click silently ignored by Cypress only if {force:true}; otherwise it will fail the actionability check) or will dismiss the editor without persisting — making the subsequent findDescriptionEditButton().click() brittle. Either add a label edit before saving, or call a cancel/close affordance to exit edit mode explicitly. Same concern for propertyRow.findSaveButton().click() on line 189.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`
at line 181, The test is clicking modelDetailsExpandedCard.findLabelSaveButton()
immediately after findLabelEditButton() without making any change, which can be
a no-op; update the test so after invoking
modelDetailsExpandedCard.findLabelEditButton().click() you either type a change
into the label input (e.g., append or set some text) before calling
modelDetailsExpandedCard.findLabelSaveButton().click() or explicitly exit edit
mode via the component’s cancel/close affordance instead of saving; apply the
same fix for propertyRow.findSaveButton() — ensure you reference the label input
or cancel control used by findLabelEditButton/findLabelSaveButton and
propertyRow.findSaveButton so the subsequent findDescriptionEditButton().click()
is not brittle.
| .eq(0) | ||
| .should('be.visible') | ||
| .within(() => { | ||
| cy.contains(/Testing label already exists|can't exceed 63 characters/g).should('exist'); |
There was a problem hiding this comment.
Drop the /g flag on regexes passed to cy.contains.
Global RegExp instances are stateful (lastIndex persists between .test()/.exec() calls). When Cypress retries cy.contains against the same regex object, matches can flip between true/false across retries, producing flaky pass/fail behavior. The flag also adds nothing here since cy.contains only needs a single match.
Proposed fix
- cy.contains(/Testing label already exists|can't exceed 63 characters/g).should('exist');
+ cy.contains(/Testing label already exists|can't exceed 63 characters/).should('exist');(apply to both occurrences at lines 275 and 282)
Also applies to: 282-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`
at line 275, The regex passed to cy.contains (used in the assertions that check
"Testing label already exists" and "can't exceed 63 characters") incorrectly
includes the global /g flag which makes the RegExp stateful and causes flaky
retries; remove the /g flag (use a plain RegExp like /Testing label already
exists|can't exceed 63 characters/ or two separate assertions/strings) for both
occurrences so cy.contains receives a non-global regex and retries behave
deterministically.
Signed-off-by: ppadti <ppadti@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts (1)
154-154: UnusedUpdatePropertyRowalias; also inconsistent casing.Line 154 aliases the model-version PATCH as
@UpdatePropertyRowbut nocy.wait('@UpdatePropertyRow')exists in this spec. Either drop the alias or add an assertion on the PATCH body for the property-edit path (symmetric with@patchRegisteredModelon line 166). While at it, align casing with the other alias (updateModelVersion/patchModelVersion) since PascalCase is inconsistent withpatchRegisteredModel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts` at line 154, The alias `@UpdatePropertyRow` is unused and uses inconsistent PascalCase; either remove that .as('UpdatePropertyRow') or rename it to camelCase (e.g., .as('updateModelVersion' or 'patchModelVersion') to match existing aliases) and add a cy.wait('@updateModelVersion') (or cy.wait('@patchModelVersion')) after the property edit that asserts the intercepted PATCH request body contains the expected property-edit path/payload (symmetric with the existing `@patchRegisteredModel` check). Locate the .as('UpdatePropertyRow') in the spec and implement one of these two options so the alias is either removed or used with a body assertion and consistent naming.
🤖 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/mocked/modelRegistry/modelDetailsEditing.cy.ts`:
- Line 247: The test currently uses
modelVersionDetails.findSaveLabelsButton().should('exist').click(), which only
asserts DOM presence and can pass for hidden or covered elements; update the
assertion to ensure the button is actionable by replacing the existence check
with visibility (and disabled-state if relevant) — e.g., use
should('be.visible') and optionally should('not.be.disabled') before invoking
modelVersionDetails.findSaveLabelsButton().click() so the click only runs when
the Save Labels button is actually interactable.
- Around line 262-275: Add an explicit length guard before asserting specific
indexes of the label-error-alert collection: after the
cy.findAllByTestId('label-error-alert') call and before the .eq(0)/.eq(1)
checks, add a should('have.length', 2) assertion so the test explicitly expects
two alerts; update the sequence around the existing
cy.findAllByTestId('label-error-alert') usages in modelDetailsEditing.cy.ts (the
block that currently calls .eq(0) and .eq(1)) to include this length check to
avoid flakiness if the component stops rendering both alerts.
---
Nitpick comments:
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`:
- Line 154: The alias `@UpdatePropertyRow` is unused and uses inconsistent
PascalCase; either remove that .as('UpdatePropertyRow') or rename it to
camelCase (e.g., .as('updateModelVersion' or 'patchModelVersion') to match
existing aliases) and add a cy.wait('@updateModelVersion') (or
cy.wait('@patchModelVersion')) after the property edit that asserts the
intercepted PATCH request body contains the expected property-edit path/payload
(symmetric with the existing `@patchRegisteredModel` check). Locate the
.as('UpdatePropertyRow') in the spec and implement one of these two options so
the alias is either removed or used with a body assertion and consistent naming.
🪄 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: cf0a3201-b236-464a-9ad4-7911ce7515df
📒 Files selected for processing (1)
packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts
| cy.contains('Second Label').parent().find('[data-testid^="remove-label-"]').click(); | ||
| }); | ||
|
|
||
| modelVersionDetails.findSaveLabelsButton().should('exist').click(); |
There was a problem hiding this comment.
.should('exist').click() is too weak before a click.
exist passes for detached/covered elements; prefer be.visible (and, if relevant, not.be.disabled) so the click is gated on the save button actually being actionable. Otherwise a regression where the button is hidden could still pass the existence check.
Proposed change
- modelVersionDetails.findSaveLabelsButton().should('exist').click();
+ modelVersionDetails.findSaveLabelsButton().should('be.visible').and('not.be.disabled').click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modelVersionDetails.findSaveLabelsButton().should('exist').click(); | |
| modelVersionDetails.findSaveLabelsButton().should('be.visible').and('not.be.disabled').click(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`
at line 247, The test currently uses
modelVersionDetails.findSaveLabelsButton().should('exist').click(), which only
asserts DOM presence and can pass for hidden or covered elements; update the
assertion to ensure the button is actionable by replacing the existence check
with visibility (and disabled-state if relevant) — e.g., use
should('be.visible') and optionally should('not.be.disabled') before invoking
modelVersionDetails.findSaveLabelsButton().click() so the click only runs when
the Save Labels button is actually interactable.
| cy.findAllByTestId('label-error-alert') | ||
| .eq(0) | ||
| .should('be.visible') | ||
| .within(() => { | ||
| cy.contains(`can't exceed 63 characters`).should('exist'); | ||
| }); | ||
|
|
||
| cy.findAllByTestId('label-error-alert') | ||
| .eq(1) | ||
| .should('be.visible') | ||
| .within(() => { | ||
| cy.contains(`can't exceed 63 characters`).should('exist'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the label-error-alert testid usages to confirm how many alerts render per invalid label.
rg -nP -C4 'label-error-alert' --type=ts --type=tsxRepository: opendatahub-io/odh-dashboard
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for label-error-alert usages without the tsx type restriction
rg -nP -C4 'label-error-alert' packages/Repository: opendatahub-io/odh-dashboard
Length of output: 10319
🏁 Script executed:
#!/bin/bash
# Read the test file to see the setup (lines 254-275) and understand what's being tested
head -n 280 packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | tail -n 40Repository: opendatahub-io/odh-dashboard
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Find component files that might contain the label-error-alert rendering logic
fd -e ts -e js -e tsx -e jsx . packages/ | xargs grep -l 'label-error-alert' | head -20Repository: opendatahub-io/odh-dashboard
Length of output: 393
🏁 Script executed:
#!/bin/bash
# Find component files that render label-error-alert (likely in the model registry frontend)
fd -e ts -e tsx -e js -e jsx . packages/model-registry packages/cypress | xargs grep -l 'label-error-alert' 2>/dev/null | grep -v test | grep -v cypressRepository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Search for where label-error-alert testid is set (likely in component code)
rg "label-error-alert" packages/model-registry packages/ --type ts --type js -C3 | grep -v "test\|cypress" | head -60Repository: opendatahub-io/odh-dashboard
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Broader search for label-error-alert without type restrictions
rg 'label-error-alert' . --no-type-list | grep -v '.cy.ts' | head -20Repository: opendatahub-io/odh-dashboard
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Find the component files in model-registry frontend
fd -type f -name "*.tsx" -o -name "*.ts" packages/model-registry/upstream/frontend/src | grep -i label | head -20Repository: opendatahub-io/odh-dashboard
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Search for EditableLabels or similar component that would render these alerts
rg -i 'editable.*label|label.*error' packages/model-registry/upstream/frontend/src --type ts --type tsx | grep -v test | head -30Repository: opendatahub-io/odh-dashboard
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for label-error-alert in all files
rg 'label-error-alert' packages/model-registry/upstream/frontend/src/ | grep -v testRepository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Find component files with "label" in the name
find packages/model-registry/upstream/frontend/src -type f \( -name "*label*" -o -name "*Label*" \) ! -path "*/node_modules/*" ! -path "*/__tests__/*"Repository: opendatahub-io/odh-dashboard
Length of output: 1213
🏁 Script executed:
#!/bin/bash
# Look at the test to understand the component context better
grep -B20 "should validate label length" packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | head -40Repository: opendatahub-io/odh-dashboard
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Search across the entire repo for label-error-alert
rg 'label-error-alert' . | grep -v cypress | grep -v testRepository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check the ModelLabels component which likely renders the editable labels
cat packages/model-registry/upstream/frontend/src/app/pages/modelRegistry/screens/components/ModelLabels.tsxRepository: opendatahub-io/odh-dashboard
Length of output: 3848
🏁 Script executed:
#!/bin/bash
# Search for editable-label-group component
rg 'editable-label-group' packages/model-registry/upstream/frontend/src/ --type tsRepository: opendatahub-io/odh-dashboard
Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Look for editable labels component more broadly
find packages/model-registry/upstream/frontend/src -name "*.ts" -o -name "*.tsx" | xargs grep -l 'editable.*label\|EditableLabel' | grep -v testRepository: opendatahub-io/odh-dashboard
Length of output: 310
🏁 Script executed:
#!/bin/bash
# Search in the entire packages directory for where label-error-alert is rendered
find packages -name "*.ts" -o -name "*.tsx" | xargs grep -l 'label-error-alert' | grep -v test | grep -v cypressRepository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check the ModelVersionDetailsView component
cat packages/model-registry/upstream/frontend/src/app/pages/modelRegistry/screens/ModelVersionDetails/ModelVersionDetailsView.tsx | head -200Repository: opendatahub-io/odh-dashboard
Length of output: 7800
🏁 Script executed:
#!/bin/bash
# Search for where editable-label-group is defined/rendered in component code
rg 'editable-label-group' packages/model-registry/upstream/frontend/src --type ts -B5 -A10 | grep -v test | head -80Repository: opendatahub-io/odh-dashboard
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Look for any component that renders the validation error alerts
rg 'error.*alert|alert.*error' packages/model-registry/upstream/frontend/src/app --type ts -l | head -10Repository: opendatahub-io/odh-dashboard
Length of output: 1226
🏁 Script executed:
#!/bin/bash
# Find where mod-arch-shared is located or if it's a package
find . -name "mod-arch-shared" -o -name "*mod-arch*" -type d | head -5Repository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check if mod-arch-shared is in node_modules or locally defined
rg "label-error-alert" . --type ts --type js 2>/dev/null | grep -v cypress | grep -v test | head -10Repository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Look at the cypress test file more carefully to see if there's a comment or pattern explaining why two alerts
cat packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | sed -n '240,280p'Repository: opendatahub-io/odh-dashboard
Length of output: 1311
🏁 Script executed:
#!/bin/bash
# Check if mod-arch-shared is a package in package.json
grep -r "mod-arch-shared" packages/*/package.json | head -5Repository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check the version and see if there's documentation or source
rg "EditableLabelsDescriptionListGroup" packages/model-registry/upstream/frontend/src --type ts -B2 -A10 | head -40Repository: opendatahub-io/odh-dashboard
Length of output: 6772
🏁 Script executed:
#!/bin/bash
# See if there are any other tests with similar patterns to understand the expected behavior
grep -A5 "findAllByTestId.*eq\(1\)" packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | head -30Repository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Check if there's a build or dist directory where mod-arch-shared might be available
find . -name "mod-arch-shared" -type d 2>/dev/null | head -5Repository: opendatahub-io/odh-dashboard
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Try to find the package.json and lockfile to see mod-arch-shared version/location
find packages/model-registry packages/cypress -name "package.json" -exec grep -l "mod-arch-shared" {} \;Repository: opendatahub-io/odh-dashboard
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Look at git history to see if this test was recently added or modified
cd packages/cypress && git log --oneline -20 -- cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts 2>/dev/null | head -10Repository: opendatahub-io/odh-dashboard
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Check what the recent fix changed in the test file
cd packages/cypress && git show cd060563b -- cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | head -100Repository: opendatahub-io/odh-dashboard
Length of output: 4358
🏁 Script executed:
#!/bin/bash
# Look at the package.json to see the mod-arch-shared dependency
cat packages/model-registry/upstream/frontend/package.json | grep -A2 "mod-arch-shared"Repository: opendatahub-io/odh-dashboard
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments in the component about the two alerts
grep -r "error-alert" packages/model-registry/upstream/frontend/src --type ts -B3 -A3 | head -50Repository: opendatahub-io/odh-dashboard
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Look at the complete test file to see if there are any comments explaining the two alerts
cat packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts | sed -n '248,315p'Repository: opendatahub-io/odh-dashboard
Length of output: 1989
🏁 Script executed:
#!/bin/bash
# Check the corresponding upstream test to see if it has the same pattern
cat packages/model-registry/upstream/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDetails.cy.ts | sed -n '490,520p'Repository: opendatahub-io/odh-dashboard
Length of output: 1119
🏁 Script executed:
#!/bin/bash
# Search for any README or documentation about EditableLabelsDescriptionListGroup
find . -name "README*" -o -name "*.md" | xargs grep -l "EditableLabelsDescriptionListGroup" 2>/dev/nullRepository: opendatahub-io/odh-dashboard
Length of output: 56
Add explicit length guard to prevent flaky test if UI alert count changes.
The test asserts both .eq(0) and .eq(1) of label-error-alert after adding a single 64-char label. While this pattern appears intentional across test files (the component likely renders two alerts per invalid label—inline and summary), the test lacks a defensive assertion. If the component ever changes to render only one alert, .eq(1) will fail intermittently, masking the actual issue. Add cy.findAllByTestId('label-error-alert').should('have.length', 2) before the existing assertions to make the expected behavior explicit and catch such regressions early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/cypress/cypress/tests/mocked/modelRegistry/modelDetailsEditing.cy.ts`
around lines 262 - 275, Add an explicit length guard before asserting specific
indexes of the label-error-alert collection: after the
cy.findAllByTestId('label-error-alert') call and before the .eq(0)/.eq(1)
checks, add a should('have.length', 2) assertion so the test explicitly expects
two alerts; update the sequence around the existing
cy.findAllByTestId('label-error-alert') usages in modelDetailsEditing.cy.ts (the
block that currently calls .eq(0) and .eq(1)) to include this length check to
avoid flakiness if the component stops rendering both alerts.
Signed-off-by: ppadti <ppadti@redhat.com>
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7338 +/- ##
==========================================
+ Coverage 64.98% 65.73% +0.75%
==========================================
Files 2446 2471 +25
Lines 76161 76722 +561
Branches 19213 19387 +174
==========================================
+ Hits 49490 50433 +943
+ Misses 26671 26289 -382
... and 145 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Closes: https://redhat.atlassian.net/browse/RHOAIENG-37600
Description
This is PR which add cypress and unit of upstream to the CI
How Has This Been Tested?
Make sure the CI is running upstream model-registry tests on CI and CI passes without any errors.
Test Impact
NA
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