fix(e2e): handle other catalog sources when verifying disable in testSourceEnableDisable#6734
Conversation
📝 WalkthroughWalkthroughThis change modifies source enable/disable testing and utilities to enhance verification robustness. The test now enforces network-level assertions for disable PATCH requests ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. 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
🤖 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/modelCatalog/testSourceEnableDisable.cy.ts`:
- Around line 68-69: After the stabilization wait, add explicit assertions that
the two disabled sources (testData.redhatAiSourceId and
testData.redhatAiSourceId2) are not rendered: locate the catalog card element
used elsewhere in this spec (the selector for model catalog cards) and assert
each of those two IDs/titles does not exist/should not be visible (e.g., use
cy.contains/ cy.get(...).should('not.exist') or should('not.be.visible') for the
cards associated with testData.redhatAiSourceId and testData.redhatAiSourceId2)
immediately after calling waitForModelCatalogAfterDisable so the test fails if
those specific sources remain displayed.
In `@packages/cypress/cypress/utils/oc_commands/modelCatalog.ts`:
- Around line 401-430: The helper currently returns early when the user
configmap yields "0" and also treats entries with missing .enabled as disabled;
instead, always fetch both configmaps (model-catalog-default-sources and
model-catalog-sources), treat missing .enabled as true, and compute the
effective enabled catalogs by overlaying user entries onto defaults (user
entries override defaults by .id). Update buildJqFilter (or the jq/python
pipeline in buildCommand) to treat (.enabled == true) OR (has no .enabled) as
enabled, or fetch raw JSON for both and in verifyModelCatalogSourceEnabled/this
function merge arrays in JS: build a map keyed by .id from defaults,
apply/replace with user entries, count entries where enabled is true or missing,
then use that count to decide which UI state to wait for; keep using
execWithOutput to get both userCmd and defaultCmd before returning a result.
- Around line 425-433: The current branch in execWithOutput handling (variables
defaultCmd, defaultResult, defaultCount) swallows oc/yq/jq failures and returns
cy.wrap(false); instead, when defaultResult.code !== 0 or parseInt produces NaN,
fail fast by rejecting/throwing an Error so waitForModelCatalogAfterDisable
doesn't proceed on a bad parse — create an error that includes a short, masked
stderr indicator (e.g., "command failed: <redacted stderr>" or include only
non-sensitive metadata, not raw stderr), and return/throw that error instead of
assuming there are no other sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bc03e579-7ea6-4150-8ccb-9718d743c805
📥 Commits
Reviewing files that changed from the base of the PR and between 0c10de9 and 4797bfbe7d067f724a145853a26c5dc23ad0a190.
📒 Files selected for processing (2)
packages/cypress/cypress/tests/e2e/modelCatalog/testSourceEnableDisable.cy.tspackages/cypress/cypress/utils/oc_commands/modelCatalog.ts
| const buildJqFilter = (): string => | ||
| `[.catalogs[] | select(.enabled == true) | select(.id | IN(${excludeFilter}) | not)] | length`; | ||
|
|
||
| const buildCommand = (configmap: string): string => { | ||
| const getYaml = `oc get configmap ${configmap} -n ${namespace} -o jsonpath='{.data.sources\\.yaml}'`; | ||
| return ` | ||
| if command -v yq >/dev/null 2>&1; then | ||
| ${getYaml} | yq -o=json | jq '${buildJqFilter()}' | ||
| else | ||
| ${getYaml} | python3 -c 'import sys, yaml, json; json.dump(yaml.safe_load(sys.stdin), sys.stdout)' 2>/dev/null | jq '${buildJqFilter()}' | ||
| fi | ||
| `.trim(); | ||
| }; | ||
|
|
||
| const userCmd = buildCommand('model-catalog-sources'); | ||
| const defaultCmd = buildCommand('model-catalog-default-sources'); | ||
|
|
||
| return execWithOutput(userCmd, 30).then((userResult: CommandLineResult) => { | ||
| const userCount = parseInt(userResult.stdout.trim(), 10); | ||
| if (userResult.code === 0 && !Number.isNaN(userCount)) { | ||
| cy.log(`Other enabled sources (user configmap): ${userCount}`); | ||
| return cy.wrap(userCount > 0); | ||
| } | ||
|
|
||
| return execWithOutput(defaultCmd, 30).then((defaultResult: CommandLineResult) => { | ||
| const defaultCount = parseInt(defaultResult.stdout.trim(), 10); | ||
| if (defaultResult.code === 0 && !Number.isNaN(defaultCount)) { | ||
| cy.log(`Other enabled sources (default configmap): ${defaultCount}`); | ||
| return cy.wrap(defaultCount > 0); | ||
| } |
There was a problem hiding this comment.
Overlay defaults and user overrides before deciding which UI state to wait for.
If model-catalog-sources is only the override layer—as verifyModelCatalogSourceEnabled() assumes earlier in this file—Line 420 turns a parseable 0 into the final answer and never consults model-catalog-default-sources. Line 402 also ignores default entries whose enabled field is omitted even though this file treats that case as implicitly enabled. This helper can still send the test down the empty-state branch on clusters that have other default-enabled catalogs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/utils/oc_commands/modelCatalog.ts` around lines 401
- 430, The helper currently returns early when the user configmap yields "0" and
also treats entries with missing .enabled as disabled; instead, always fetch
both configmaps (model-catalog-default-sources and model-catalog-sources), treat
missing .enabled as true, and compute the effective enabled catalogs by
overlaying user entries onto defaults (user entries override defaults by .id).
Update buildJqFilter (or the jq/python pipeline in buildCommand) to treat
(.enabled == true) OR (has no .enabled) as enabled, or fetch raw JSON for both
and in verifyModelCatalogSourceEnabled/this function merge arrays in JS: build a
map keyed by .id from defaults, apply/replace with user entries, count entries
where enabled is true or missing, then use that count to decide which UI state
to wait for; keep using execWithOutput to get both userCmd and defaultCmd before
returning a result.
| return execWithOutput(defaultCmd, 30).then((defaultResult: CommandLineResult) => { | ||
| const defaultCount = parseInt(defaultResult.stdout.trim(), 10); | ||
| if (defaultResult.code === 0 && !Number.isNaN(defaultCount)) { | ||
| cy.log(`Other enabled sources (default configmap): ${defaultCount}`); | ||
| return cy.wrap(defaultCount > 0); | ||
| } | ||
|
|
||
| cy.log('Could not determine other enabled sources, assuming none'); | ||
| return cy.wrap(false); |
There was a problem hiding this comment.
Do not convert lookup/parsing failures into “no other sources.”
Lines 432-433 collapse oc/yq/jq failures into false, so waitForModelCatalogAfterDisable() takes the empty-state path on infrastructure/parsing errors and only fails later with a misleading symptom. Fail here with masked stderr instead of assuming “none”.
Suggested fix
- cy.log('Could not determine other enabled sources, assuming none');
- return cy.wrap(false);
+ const maskedStderr = maskSensitiveInfo(
+ [userResult.stderr, defaultResult.stderr].filter(Boolean).join('\n'),
+ );
+ throw new Error(`Could not determine other enabled sources: ${maskedStderr}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/utils/oc_commands/modelCatalog.ts` around lines 425
- 433, The current branch in execWithOutput handling (variables defaultCmd,
defaultResult, defaultCount) swallows oc/yq/jq failures and returns
cy.wrap(false); instead, when defaultResult.code !== 0 or parseInt produces NaN,
fail fast by rejecting/throwing an Error so waitForModelCatalogAfterDisable
doesn't proceed on a bad parse — create an error that includes a short, masked
stderr indicator (e.g., "command failed: <redacted stderr>" or include only
non-sensitive metadata, not raw stderr), and return/throw that error instead of
assuming there are no other sources.
| }; | ||
|
|
||
| const userCmd = buildCommand('model-catalog-sources'); | ||
| const defaultCmd = buildCommand('model-catalog-default-sources'); |
There was a problem hiding this comment.
Does this work on both Upstream and Downstream?
|
/retest-required |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6734 +/- ##
==========================================
- Coverage 64.81% 64.80% -0.02%
==========================================
Files 2441 2441
Lines 75996 75996
Branches 19158 19158
==========================================
- Hits 49257 49247 -10
- Misses 26739 26749 +10 see 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
antowaddle
left a comment
antowaddle
left a comment
There was a problem hiding this comment.
Can we also see the results from the local/Jenkins run here too when we have?
| const buildJqFilter = (): string => | ||
| `[.catalogs[] | select(.enabled == true) | select(.id | IN(${excludeFilter}) | not)] | length`; | ||
|
|
||
| const buildCommand = (configmap: string): string => { |
There was a problem hiding this comment.
Is there a simpler way to do this? My concern here is that the pythons commands additionally may also fail when executed on CI/Jenkins.
| const buildCommand = (configmap: string): string => { | ||
| const getYaml = `oc get configmap ${configmap} -n ${namespace} -o jsonpath='{.data.sources\\.yaml}'`; | ||
| return ` | ||
| if command -v yq >/dev/null 2>&1; then | ||
| ${getYaml} | yq -o=json | jq '${buildJqFilter()}' | ||
| else | ||
| ${getYaml} | python3 -c 'import sys, yaml, json; json.dump(yaml.safe_load(sys.stdin), sys.stdout)' 2>/dev/null | jq '${buildJqFilter()}' | ||
| fi | ||
| `.trim(); |
There was a problem hiding this comment.
Building on @antowaddle's question about simplification: since js-yaml is already a dependency (it's imported in the test file), you could eliminate the yq/python3/jq pipeline entirely by fetching the raw YAML with oc get and doing the parsing + filtering in TypeScript:
const cmd = `oc get configmap model-catalog-sources -n ${namespace} -o jsonpath='{.data.sources\\.yaml}'`;
return execWithOutput(cmd, 30).then((result: CommandLineResult) => {
if (result.code !== 0) {
// fall back to defaults configmap...
}
const parsed = yaml.load(result.stdout) as { catalogs: Array<{ id: string; enabled?: boolean }> };
const otherEnabled = parsed.catalogs.filter(
(c) => (c.enabled !== false) && !excludeSourceIds.includes(c.id),
);
return cy.wrap(otherEnabled.length > 0);
});This approach:
- Removes the dependency on
yq,python3, andjqbeing available in CI - Removes the
2>/dev/nullthat silently swallows python3 errors - Handles the implicit-enabled case correctly (
enabled !== falseinstead ofenabled == true, so sources without an.enabledfield are treated as enabled) - Makes the logic easier to read and maintain
| * @param maxAttempts Maximum number of attempts (default: 20) | ||
| * @param pollIntervalMs Interval between attempts in milliseconds (default: 5000) |
There was a problem hiding this comment.
Nit: the JSDoc documents maxAttempts and pollIntervalMs parameters, but the function signature only has disabledSourceIds. These are leftover from a copy-paste and should be removed to avoid confusion.
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go (1)
233-250: No backoff between retry attempts.Immediate retries on conflict may not resolve contention under concurrent writes. Consider adding a small jittered backoff (e.g.,
time.Sleep(time.Duration(attempt*50) * time.Millisecond)) to reduce collision probability.Proposed fix: add backoff
+import ( + "time" + "math/rand" +) for attempt := 1; attempt <= maxConflictRetries; attempt++ { result, err := r.tryUpdateCatalogSourceConfig(ctx, client, namespace, sourceId, payload) if err == nil { return result, nil } if !errors.Is(err, ErrCatalogSourceConflict) { return nil, err } lastErr = err if attempt < maxConflictRetries { + // Jittered backoff before retry + backoff := time.Duration(attempt*50+rand.Intn(50)) * time.Millisecond + time.Sleep(backoff) if sessionLogger, ok := ctx.Value(constants.TraceLoggerKey).(*slog.Logger); ok && sessionLogger != nil { sessionLogger.Warn("configmap conflict detected, retrying",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go` around lines 233 - 250, The retry loop in the caller of tryUpdateCatalogSourceConfig retries immediately on ErrCatalogSourceConflict; add a small jittered backoff between attempts to reduce contention. After detecting a conflict (when errors.Is(err, ErrCatalogSourceConflict) and before the next loop iteration), sleep for a short duration that grows with attempt (e.g., attempt*baseMs) plus random jitter using math/rand, and seed or use rand.Intn for jitter; keep this only for attempt < maxConflictRetries and log the chosen backoff via the sessionLogger (from ctx.Value(constants.TraceLoggerKey)). Ensure imports for time and math/rand are added and that the logic references maxConflictRetries, attempt, ErrCatalogSourceConflict, and tryUpdateCatalogSourceConfig.
🤖 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/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go`:
- Around line 231-252: The code unsafely type-asserts the context logger with
ctx.Value(constants.TraceLoggerKey).(*slog.Logger) inside the retry loop, which
can panic if the key is missing or holds a different type; change this to use
the comma-ok idiom (e.g., val, ok :=
ctx.Value(constants.TraceLoggerKey).(*slog.Logger)) and handle the false case by
falling back to a safe logger (global/default or a no-op logger) before calling
sessionLogger.Warn; update the block around the call to
tryUpdateCatalogSourceConfig and the ErrCatalogSourceConflict retry handling to
use the validated/fallback sessionLogger so no runtime panic occurs.
---
Nitpick comments:
In
`@packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go`:
- Around line 233-250: The retry loop in the caller of
tryUpdateCatalogSourceConfig retries immediately on ErrCatalogSourceConflict;
add a small jittered backoff between attempts to reduce contention. After
detecting a conflict (when errors.Is(err, ErrCatalogSourceConflict) and before
the next loop iteration), sleep for a short duration that grows with attempt
(e.g., attempt*baseMs) plus random jitter using math/rand, and seed or use
rand.Intn for jitter; keep this only for attempt < maxConflictRetries and log
the chosen backoff via the sessionLogger (from
ctx.Value(constants.TraceLoggerKey)). Ensure imports for time and math/rand are
added and that the logic references maxConflictRetries, attempt,
ErrCatalogSourceConflict, and tryUpdateCatalogSourceConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3d02d7fc-ce63-4e5e-a539-2fe8091fb2da
📥 Commits
Reviewing files that changed from the base of the PR and between 715e4f075675537c0c9facb19ec6c35fa34e22fc and 97d2a3d33ab7495ca90ff22b60d8ad76dc2495b6.
📒 Files selected for processing (1)
packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go
| ) (*models.CatalogSourceConfig, error) { | ||
| var lastErr error | ||
| for attempt := 1; attempt <= maxConflictRetries; attempt++ { | ||
| result, err := r.tryUpdateCatalogSourceConfig(ctx, client, namespace, sourceId, payload) | ||
| if err == nil { | ||
| return result, nil | ||
| } | ||
| if !errors.Is(err, ErrCatalogSourceConflict) { | ||
| return nil, err | ||
| } | ||
| lastErr = err | ||
| if attempt < maxConflictRetries { | ||
| sessionLogger := ctx.Value(constants.TraceLoggerKey).(*slog.Logger) | ||
| sessionLogger.Warn("configmap conflict detected, retrying", | ||
| "sourceId", sourceId, | ||
| "attempt", attempt, | ||
| "maxRetries", maxConflictRetries, | ||
| ) | ||
| } | ||
| } | ||
| return nil, lastErr | ||
| } |
There was a problem hiding this comment.
Unsafe type assertion on context value will panic if logger is absent.
Line 243: ctx.Value(constants.TraceLoggerKey).(*slog.Logger) performs an unchecked type assertion. If the context key is missing or holds a different type, this causes a runtime panic (CWE-476: NULL Pointer Dereference equivalent in Go).
Proposed fix: use comma-ok idiom
if attempt < maxConflictRetries {
- sessionLogger := ctx.Value(constants.TraceLoggerKey).(*slog.Logger)
- sessionLogger.Warn("configmap conflict detected, retrying",
- "sourceId", sourceId,
- "attempt", attempt,
- "maxRetries", maxConflictRetries,
- )
+ if sessionLogger, ok := ctx.Value(constants.TraceLoggerKey).(*slog.Logger); ok && sessionLogger != nil {
+ sessionLogger.Warn("configmap conflict detected, retrying",
+ "sourceId", sourceId,
+ "attempt", attempt,
+ "maxRetries", maxConflictRetries,
+ )
+ }
}📝 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.
| ) (*models.CatalogSourceConfig, error) { | |
| var lastErr error | |
| for attempt := 1; attempt <= maxConflictRetries; attempt++ { | |
| result, err := r.tryUpdateCatalogSourceConfig(ctx, client, namespace, sourceId, payload) | |
| if err == nil { | |
| return result, nil | |
| } | |
| if !errors.Is(err, ErrCatalogSourceConflict) { | |
| return nil, err | |
| } | |
| lastErr = err | |
| if attempt < maxConflictRetries { | |
| sessionLogger := ctx.Value(constants.TraceLoggerKey).(*slog.Logger) | |
| sessionLogger.Warn("configmap conflict detected, retrying", | |
| "sourceId", sourceId, | |
| "attempt", attempt, | |
| "maxRetries", maxConflictRetries, | |
| ) | |
| } | |
| } | |
| return nil, lastErr | |
| } | |
| ) (*models.CatalogSourceConfig, error) { | |
| var lastErr error | |
| for attempt := 1; attempt <= maxConflictRetries; attempt++ { | |
| result, err := r.tryUpdateCatalogSourceConfig(ctx, client, namespace, sourceId, payload) | |
| if err == nil { | |
| return result, nil | |
| } | |
| if !errors.Is(err, ErrCatalogSourceConflict) { | |
| return nil, err | |
| } | |
| lastErr = err | |
| if attempt < maxConflictRetries { | |
| if sessionLogger, ok := ctx.Value(constants.TraceLoggerKey).(*slog.Logger); ok && sessionLogger != nil { | |
| sessionLogger.Warn("configmap conflict detected, retrying", | |
| "sourceId", sourceId, | |
| "attempt", attempt, | |
| "maxRetries", maxConflictRetries, | |
| ) | |
| } | |
| } | |
| } | |
| return nil, lastErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/model-registry/upstream/bff/internal/repositories/model_catalog_settings.go`
around lines 231 - 252, The code unsafely type-asserts the context logger with
ctx.Value(constants.TraceLoggerKey).(*slog.Logger) inside the retry loop, which
can panic if the key is missing or holds a different type; change this to use
the comma-ok idiom (e.g., val, ok :=
ctx.Value(constants.TraceLoggerKey).(*slog.Logger)) and handle the false case by
falling back to a safe logger (global/default or a no-op logger) before calling
sessionLogger.Warn; update the block around the call to
tryUpdateCatalogSourceConfig and the ErrCatalogSourceConflict retry handling to
use the validated/fallback sessionLogger so no runtime panic occurs.
…SourceEnableDisable
…source update" This reverts commit 97d2a3d33ab7495ca90ff22b60d8ad76dc2495b6.
…d correct configmap precedence
manaswinidas
force-pushed
the
tests-e2e-catalog-enable-disable
branch
from
70964cc to
aa35d9a
Compare
|
/retest |
|
/retest-required |
|
Still need to test this locally - it's failing on Jenkins incrementally on all the above commits |
|
/retest-required |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/cypress/cypress/tests/e2e/modelCatalog/testSourceEnableDisable.cy.ts (2)
62-75: Scope network assertions to the specific source being toggled.Line 62 uses a wildcard intercept for all source-config PATCH calls. Line 66 and Line 74 only assert
200, so an unrelated PATCH can satisfy@toggleSourceand mask a wrong request association.Proposed diff
- cy.intercept('PATCH', '**/source_configs/*').as('toggleSource'); + cy.intercept('PATCH', `**/source_configs/${testData.redhatAiSourceId}`).as( + 'toggleSourceRedHat1', + ); + cy.intercept('PATCH', `**/source_configs/${testData.redhatAiSourceId2}`).as( + 'toggleSourceRedHat2', + ); cy.step(`Disable the ${testData.sourceName} source`); modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId).click(); - cy.wait('@toggleSource').its('response.statusCode').should('eq', 200); + cy.wait('@toggleSourceRedHat1').its('response.statusCode').should('eq', 200); modelCatalogSettings.findToggleAlert().should('not.exist'); cy.step(`Disable the ${testData.sourceName2} source`); modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId2).click(); - cy.wait('@toggleSource').its('response.statusCode').should('eq', 200); + cy.wait('@toggleSourceRedHat2').its('response.statusCode').should('eq', 200); modelCatalogSettings.findToggleAlert().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/modelCatalog/testSourceEnableDisable.cy.ts` around lines 62 - 75, The test uses a wildcard intercept alias toggleSource so unrelated PATCHes can satisfy cy.wait and hide mismatches; scope the network assertion to the specific source by verifying the request targets the expected source id after each toggle. After clicking modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId) wait on '@toggleSource' and assert the intercepted request (its('request.url') or its('request.body')) includes testData.redhatAiSourceId before asserting response.statusCode is 200; repeat the same pattern for testData.redhatAiSourceId2 (or register separate intercepts/aliases that include the id) and keep verifyModelCatalogSourceEnabled(testData.redhatAiSourceId, false) unchanged.
23-27: Restore original source states instead of forcing enabled state as a permanent side effect.Line 24-Line 27 unconditionally mutates shared cluster state, and the spec’s cleanup path keeps both sources enabled rather than restoring their pre-test values. Capture initial enabled/disabled values and restore them in
after()to avoid cross-spec state leakage.As per coding guidelines,
packages/cypress/**/*.{ts,js}: "Tests must be idempotent: runnable in any order without shared state."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/modelCatalog/testSourceEnableDisable.cy.ts` around lines 23 - 27, The test currently forces both sources enabled by calling enableModelCatalogSource(testData.redhatAiSourceId) and enableModelCatalogSource(testData.redhatAiSourceId2) and leaves them enabled in cleanup; instead, capture each source's initial enabled state (e.g., in before() or at test start into variables like initialEnabled1/initialEnabled2), use enableModelCatalogSource(...) for the test actions and verifyModelCatalogSourceEnabled(...) during the test, and then in after() restore each source to its original state by calling enableModelCatalogSource(...) or the appropriate toggle with the captured boolean values for testData.redhatAiSourceId and testData.redhatAiSourceId2 so the spec is idempotent and does not leak state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/cypress/cypress/tests/e2e/modelCatalog/testSourceEnableDisable.cy.ts`:
- Around line 62-75: The test uses a wildcard intercept alias toggleSource so
unrelated PATCHes can satisfy cy.wait and hide mismatches; scope the network
assertion to the specific source by verifying the request targets the expected
source id after each toggle. After clicking
modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId) wait on
'@toggleSource' and assert the intercepted request (its('request.url') or
its('request.body')) includes testData.redhatAiSourceId before asserting
response.statusCode is 200; repeat the same pattern for
testData.redhatAiSourceId2 (or register separate intercepts/aliases that include
the id) and keep verifyModelCatalogSourceEnabled(testData.redhatAiSourceId,
false) unchanged.
- Around line 23-27: The test currently forces both sources enabled by calling
enableModelCatalogSource(testData.redhatAiSourceId) and
enableModelCatalogSource(testData.redhatAiSourceId2) and leaves them enabled in
cleanup; instead, capture each source's initial enabled state (e.g., in before()
or at test start into variables like initialEnabled1/initialEnabled2), use
enableModelCatalogSource(...) for the test actions and
verifyModelCatalogSourceEnabled(...) during the test, and then in after()
restore each source to its original state by calling
enableModelCatalogSource(...) or the appropriate toggle with the captured
boolean values for testData.redhatAiSourceId and testData.redhatAiSourceId2 so
the spec is idempotent and does not leak state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 889cd5f0-bb45-4177-ad31-05b08db49198
📥 Commits
Reviewing files that changed from the base of the PR and between 4797bfbe7d067f724a145853a26c5dc23ad0a190 and a11f9a6.
📒 Files selected for processing (3)
packages/cypress/cypress/pages/modelCatalogSettings.tspackages/cypress/cypress/tests/e2e/modelCatalog/testSourceEnableDisable.cy.tspackages/cypress/cypress/utils/oc_commands/modelCatalog.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cypress/cypress/pages/modelCatalogSettings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cypress/cypress/utils/oc_commands/modelCatalog.ts
mturley
left a comment
mturley
left a comment
There was a problem hiding this comment.
Hey, @manaswinidas apologies for the big AI review dump, please let me know if any of these aren't accurate. It seems like there might be some real issues here though. A few things from old reviews are unaddressed too, so some of this might be repetitive.
🤖 The core fix is sound, but I'd strongly recommend addressing the inline comments before merging to avoid potential false negatives and ensure the test actually validates what it intends to. The TypeScript rewrite is not just a style preference—it fixes real semantic bugs and makes the code more portable.
| const excludeFilter = excludeSourceIds.map((id) => `"${id}"`).join(', '); | ||
|
|
||
| const buildJqFilter = (): string => | ||
| `[.catalogs[] | select(.enabled == true) | select(.id | IN(${excludeFilter}) | not)] | length`; |
There was a problem hiding this comment.
The issue feels significant, not sure if it's necessary to rewrite in TS but it may be cleaner.
🤖 🔴 Critical: Incorrect enabled-by-default semantics
The jq filter select(.enabled == true) only counts sources that explicitly have enabled: true. According to the model catalog conventions, sources with a missing .enabled field should be treated as enabled by default. This means your helper could return false on clusters that actually have other enabled sources (just using implicit defaults).
Suggested fix: Rewrite to use js-yaml (already a dependency) instead of the shell pipeline:
const parseAndCount = (yamlContent: string): boolean => {
const parsed = yaml.load(yamlContent) as {
catalogs: Array<{ id: string; enabled?: boolean }>
};
const otherEnabled = parsed.catalogs.filter(
(c) => (c.enabled !== false) && !excludeSourceIds.includes(c.id),
);
cy.log(`Other enabled sources found: ${otherEnabled.length}`);
return otherEnabled.length > 0;
};
const userCmd = `oc get configmap model-catalog-sources -n ${namespace} -o jsonpath='{.data.sources\\.yaml}'`;
const defaultCmd = `oc get configmap model-catalog-default-sources -n ${namespace} -o jsonpath='{.data.sources\\.yaml}'`;
return execWithOutput(userCmd, 30).then((userResult: CommandLineResult) => {
if (userResult.code === 0 && userResult.stdout.trim()) {
return cy.wrap(parseAndCount(userResult.stdout));
}
return execWithOutput(defaultCmd, 30).then((defaultResult: CommandLineResult) => {
if (defaultResult.code !== 0 || !defaultResult.stdout.trim()) {
const maskedStderr = maskSensitiveInfo(
[userResult.stderr, defaultResult.stderr].filter(Boolean).join('\n')
);
throw new Error(`Failed to read catalog sources configmaps: ${maskedStderr}`);
}
return cy.wrap(parseAndCount(defaultResult.stdout));
});
});This also fixes:
- ✅ Portability: no dependency on
yq/python3/jqin CI - ✅ Error handling: fails fast instead of assuming "no other sources"
- ✅ Simpler code: single language instead of shell + jq
| } | ||
|
|
||
| cy.log('Could not determine other enabled sources, assuming none'); | ||
| return cy.wrap(false); |
There was a problem hiding this comment.
Feels maybe significant but I'm not sure if Claude is right about "Critical".
🤖 🔴 Critical: Silent failure handling
Lines 446-447 log "assuming none" and return false when parsing fails. This masks infrastructure errors and could cause misleading test failures.
Fix: This would be addressed by the TypeScript rewrite suggestion above, which throws an error with masked stderr instead of assuming "no other sources".
|
|
||
| cy.step('Verify model catalog shows empty state'); | ||
| modelCatalog.findModelCatalogEmptyState().should('exist'); | ||
| cy.step('Wait for catalog to reflect disabled sources'); |
There was a problem hiding this comment.
This one feels significant. I think waitForModelCatalogAfterDisable is missing a check.
🤖 🔴 Critical: Missing assertion for specific source absence
The test waits for UI stabilization but doesn't explicitly verify that the two disabled sources are no longer visible. If other sources are present, the test passes as long as some cards appear—even if the disabled sources are still incorrectly showing.
Suggested addition after line 83:
cy.step('Wait for catalog to reflect disabled sources');
waitForModelCatalogAfterDisable([testData.redhatAiSourceId, testData.redhatAiSourceId2]);
// Add this:
cy.step('Verify disabled sources are not visible');
modelCatalog.findModelCatalogCards().should('exist').then(($cards) => {
const visibleCards = $cards.toArray();
const visibleSourceNames = visibleCards.map(card =>
Cypress.$(card).text()
);
expect(visibleSourceNames.join(',')).not.to.include(testData.sourceName);
expect(visibleSourceNames.join(',')).not.to.include(testData.sourceName2);
});| @@ -53,23 +59,29 @@ describe('Verify Model Catalog Source Enable/Disable', () => { | |||
| cy.step('Navigate back to Model catalog settings'); | |||
| modelCatalogSettings.visit(); | |||
|
|
|||
There was a problem hiding this comment.
This one feels like it could be ignored.
🤖 🟡 Medium: Scope network intercepts to specific sources
The wildcard intercept '**/source_configs/*' could match unrelated PATCH requests. More specific matching would be safer:
cy.intercept('PATCH', `**/source_configs/${testData.redhatAiSourceId}`).as('toggleSource1');
cy.intercept('PATCH', `**/source_configs/${testData.redhatAiSourceId2}`).as('toggleSource2');
cy.step(`Disable the ${testData.sourceName} source`);
modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId).click();
cy.wait('@toggleSource1').its('response.statusCode').should('eq', 200);
// ...
cy.step(`Disable the ${testData.sourceName2} source`);
modelCatalogSettings.findEnableToggle(testData.redhatAiSourceId2).click();
cy.wait('@toggleSource2').its('response.statusCode').should('eq', 200);| * If other sources are still enabled, waits for cards to be present (no empty state). | ||
| * If no other sources remain, waits for the empty state to appear. | ||
| * @param disabledSourceIds The source IDs that were just disabled | ||
| * @param maxAttempts Maximum number of attempts (default: 20) |
There was a problem hiding this comment.
📝 Minor: JSDoc parameter mismatch
The JSDoc mentions maxAttempts and pollIntervalMs parameters that don't exist in the function signature. Remove these from the doc comment.
4 participants
Fix for e2e test failure today
The test previously assumed disabling the two Red Hat AI sources would always produce an empty catalog state. On clusters with additional catalog sources still enabled, this assertion fails. The test now checks for remaining enabled sources and expects either cards (if others exist) or empty state (if none remain).
Thread for context
This is the exact test that was failing(see screenshot) and the PR attempts to fix it
Description
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