e2e test for vLLM on MaaS#7004
e2e test for vLLM on MaaS#7004openshift-merge-bot[bot] merged 14 commits intoopendatahub-io:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughAdds legacy LLM serving test support: new fixture fields for legacy model/location/hardware, extends the DataScienceProjectData type with three optional legacy properties, introduces a KServe Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Security and code qualityActionable issues only:
No other actionable security or code-quality issues identified from the diff. 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7004 +/- ##
==========================================
+ Coverage 64.80% 64.96% +0.15%
==========================================
Files 2441 2447 +6
Lines 75996 76137 +141
Branches 19158 19205 +47
==========================================
+ Hits 49253 49464 +211
+ Misses 26743 26673 -70 see 94 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cypress/cypress/types.ts (1)
298-298: KeeplegacyServingRuntimeout of the baseDataScienceProjectDatacontract.
loadDSPFixture()casts many unrelated YAML fixtures to this shared type without schema validation. Making this LLMD-only field required tells TypeScript every project fixture has it, which is a false guarantee. Make it optional here or require it through an LLMD-specific subtype in the spec that needs it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/types.ts` at line 298, The DataScienceProjectData base type wrongly requires legacyServingRuntime, which forces unrelated fixtures cast via loadDSPFixture() to include an LLMD-only field; update the DataScienceProjectData definition to make legacyServingRuntime optional (e.g., legacyServingRuntime?: string) or remove it from the base and add an LLMD-specific subtype/interface that extends DataScienceProjectData and introduces legacyServingRuntime as required, then update any LLMD-specific consumers to use that subtype while leaving loadDSPFixture() and other generic consumers typed against the base DataScienceProjectData.
🤖 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/dataScienceProjects/models/testDeployLLMDServing.cy.ts`:
- Around line 322-331: The test currently only confirms the legacy row rendered
via findModelServerDeployedName(legacyModelName) but opens the edit dialog too
soon; before clicking the kebab/edit actions, call
checkLLMInferenceServiceState(`@legacyResourceName`, { checkReady: true }) (the
same helper used elsewhere) to wait for the inference service to reach Ready,
then proceed to getDeploymentRow(legacyModelName).findKebab().click() and
inferenceServiceActions.findEditInferenceServiceAction().click(); keep the
existing findModelServerDeployedName call or replace it with the readiness check
as preferred.
- Around line 304-311: This block duplicates CSS-class branching to detect a
disabled hardware profile; replace it by calling the existing page-object helper
selectPotentiallyDisabledProfile() on modelServingWizard and pass
hardwareProfileResourceName as used earlier, removing checks for pf-m-disabled
and prop('disabled') and the manual click logic; locate the code around
findHardProfileSelection() in the test and swap it for
modelServingWizard.selectPotentiallyDisabledProfile(hardwareProfileResourceName)
so the test uses the data-testid-aware helper instead of CSS-class branching.
---
Nitpick comments:
In `@packages/cypress/cypress/types.ts`:
- Line 298: The DataScienceProjectData base type wrongly requires
legacyServingRuntime, which forces unrelated fixtures cast via loadDSPFixture()
to include an LLMD-only field; update the DataScienceProjectData definition to
make legacyServingRuntime optional (e.g., legacyServingRuntime?: string) or
remove it from the base and add an LLMD-specific subtype/interface that extends
DataScienceProjectData and introduces legacyServingRuntime as required, then
update any LLMD-specific consumers to use that subtype while leaving
loadDSPFixture() and other generic consumers typed against the base
DataScienceProjectData.
🪄 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
Run ID: 761175e4-bbe1-4f71-8469-7990529b4c02
📒 Files selected for processing (3)
packages/cypress/cypress/fixtures/e2e/dataScienceProjects/testDeployLLMDServing.yamlpackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/types.ts
|
I think you're missing the main test. Selecting the legacy checkbox and the LLMInferenceServiceConfig serving runtime. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts (1)
157-162:⚠️ Potential issue | 🟠 MajorVerify the MaaS config resource after submit.
checkInferenceServiceState()is the only backend assertion here. If the flow creates the serving resource but never creates the pairedLLMInferenceServiceConfig, this test still goes green and misses the regression this PR is supposed to catch. Add an assertion for theLLMInferenceServiceConfigtied to this deployment, including the selectedservingRuntime, before continuing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts` around lines 157 - 162, The test currently only calls checkInferenceServiceState(resourceName, projectName, { checkReady: true }) and misses asserting the paired LLMInferenceServiceConfig; add a backend assertion after obtaining resourceName to fetch and assert the LLMInferenceServiceConfig for that deployment (e.g., query the LLMInferenceServiceConfig tied to resourceName and projectName), verify it exists and that its servingRuntime equals the selected serving runtime used in the test, and only then continue the test flow; reference the existing variables resourceName and projectName and use the same selected servingRuntime value used during model creation when asserting.
🤖 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/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts`:
- Around line 133-140: The disabled branch currently only logs and continues,
causing false positives; update the handling in
modelServingWizard.findHardProfileSelection().then(...) so that when
$el.prop('disabled') || $el.hasClass('pf-m-disabled') you assert the preselected
value equals hardwareProfileResourceName before proceeding. Concretely, after
detecting disabled, query the same control (e.g.
cy.findByTestId(hardwareProfileResourceName) or assert on $el.text()/value) and
add an assertion that it contains the expected hardwareProfileResourceName
string, then continue; leave the enabled branch behavior unchanged.
- Around line 51-55: The hardwareProfileResourceName is shared across runs
causing race conditions; generate a run-unique hardware profile name (e.g.,
append the same uuid used for projectName) and assign it to
hardwareProfileResourceName so the spec uses a per-run identifier; update calls
that create/cleanup/select hardware profiles (createCleanHardwareProfile(),
cleanupHardwareProfiles(), and any selection logic that references
hardwareProfileResourceName) to use this generated value so creation and
deletion are tied to the same unique name.
In `@packages/cypress/cypress/utils/oc_commands/llmInferenceServiceConfig.ts`:
- Line 57: The ocCommand construction uses unsanitized interpolation of
configName (and applicationNamespace) into the shell string (ocCommand = `oc get
LLMInferenceServiceConfig ${configName} -n ${applicationNamespace} -o json`),
creating a command-injection risk; fix it by validating or escaping configName
(and applicationNamespace) before usage or switch to a safe exec API (e.g.,
execFile/spawn with args) so the values are passed as separate arguments instead
of interpolated into the shell. Locate the ocCommand creation in
llmInferenceServiceConfig.ts and either enforce a stricter allowlist/regex on
configName (and namespace) or refactor the call that runs ocCommand to use
argument-array execution to eliminate shell interpolation.
- Line 4: The code reads Cypress.env('APPLICATIONS_NAMESPACE') into the
applicationNamespace constant without validation; add a guard that checks
applicationNamespace after retrieval and either throw a clear error (or set a
sensible default) so commands never get "-n undefined". Locate the
applicationNamespace declaration and the places that build oc commands using it
(the applicationNamespace variable) and update to validate: if
applicationNamespace is falsy then throw an Error with a descriptive message or
assign a documented fallback value, ensuring any oc command builders
reject/abort when the env var is missing.
- Around line 60-62: Remove the redundant expect(result.code).to.equal(0) since
cy.exec was called with { failOnNonZeroExit: true }, and protect the
JSON.parse(result.stdout) by wrapping it in a try-catch: attempt
JSON.parse(result.stdout) into config, and on parse error throw or fail with a
clear message that includes the ocCommand and result.stdout (or use
Cypress.fail/assert.fail) so the test reports a descriptive failure instead of
an uncaught SyntaxError; refer to the cy.exec call, the ocCommand variable, the
result object, and the config assignment to locate where to change the code.
- Around line 38-44: createCleanLLMInferenceServiceConfig currently passes a
file path to cleanupLLMInferenceServiceConfig which expects a resource name;
change the implementation to derive the resource name from the YAML before
calling cleanupLLMInferenceServiceConfig (or accept a separate configName
param). Specifically, in createCleanLLMInferenceServiceConfig, read/parse the
YAML at configYamlPath to extract metadata.name (or accept a configName
argument), pass that name into cleanupLLMInferenceServiceConfig, then proceed to
call createCustomResource(applicationNamespace, configYamlPath) as before; keep
references to createCleanLLMInferenceServiceConfig,
cleanupLLMInferenceServiceConfig, and createCustomResource to locate the change.
- Line 16: The ocCommand string construction interpolates unsanitized configName
(and applicationNamespace) into a shell pipeline, creating a command injection
risk; update the code that builds ocCommand (the ocCommand variable in
llmInferenceServiceConfig.ts) to avoid direct shell interpolation by either (a)
calling oc and jq with argument arrays (spawn/execFile) instead of a single
shell string, or (b) passing the raw JSON output into a Node JSON parser and
performing the filter in-process, or (c) if staying with jq, use jq's --arg to
pass configName safely; additionally validate/escape configName (e.g., allow
only expected characters) before use and ensure applicationNamespace is likewise
validated.
---
Outside diff comments:
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts`:
- Around line 157-162: The test currently only calls
checkInferenceServiceState(resourceName, projectName, { checkReady: true }) and
misses asserting the paired LLMInferenceServiceConfig; add a backend assertion
after obtaining resourceName to fetch and assert the LLMInferenceServiceConfig
for that deployment (e.g., query the LLMInferenceServiceConfig tied to
resourceName and projectName), verify it exists and that its servingRuntime
equals the selected serving runtime used in the test, and only then continue the
test flow; reference the existing variables resourceName and projectName and use
the same selected servingRuntime value used during model creation when
asserting.
🪄 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
Run ID: 9c8a21ee-47a8-45a9-80ee-2808a902044c
📒 Files selected for processing (6)
packages/cypress/cypress/fixtures/e2e/dataScienceProjects/testSingleModelAdminCreation.yamlpackages/cypress/cypress/fixtures/resources/modelServing/llmd-inference-service-config.yamlpackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.tspackages/cypress/cypress/types.tspackages/cypress/cypress/utils/oc_commands/llmInferenceServiceConfig.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cypress/cypress/fixtures/e2e/dataScienceProjects/testSingleModelAdminCreation.yaml
- packages/cypress/cypress/fixtures/resources/modelServing/llmd-inference-service-config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cypress/cypress/types.ts
- packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
from
eaf9a0a to
533dc9f
Compare
emilys314
left a comment
emilys314
left a comment
There was a problem hiding this comment.
So the main thing is we need to create a new e2e test file under the packages/cypress/cypress/tests/e2e/dataScienceProjects/models folder.
Call it something like testVLLMOnLLMnferenceService or testVLLMOnMaaS
It will have to select the "Generative" model type. Then on step 2 select the vLLM CPU LLMInferenceServiceConfig model resource that you created.
Once submit is clicked, verify that an LLMInferenceService and LLMInferenceServiceConfig resources are created in the project namespace
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.ts (1)
145-147: Hardcoded container image duplicates fixture value.
'quay.io/pierdipi/vllm-cpu:latest'is hardcoded here and in the YAML fixture. Extract to a constant or read from fixture to avoid drift.Proposed fix
+const llmInferenceServiceConfigContainerImage = 'quay.io/pierdipi/vllm-cpu:latest'; // ... checkLLMInferenceServiceConfigState(llmInferenceServiceConfigName, projectName, { - containerImage: 'quay.io/pierdipi/vllm-cpu:latest', + containerImage: llmInferenceServiceConfigContainerImage, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.ts` around lines 145 - 147, The test hardcodes the container image string in checkLLMInferenceServiceConfigState, duplicating the YAML fixture value; update the test to source that image from a single place by either defining a shared constant (e.g., export IMAGE_VLLM_CPU and use it in this test and the fixture) or by reading the fixture value at runtime and passing it into checkLLMInferenceServiceConfigState(llmInferenceServiceConfigName, projectName, { containerImage }), ensuring references to the image are only changed in one place and using the existing symbols llmInferenceServiceConfigName and projectName to locate the call.
🤖 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/dataScienceProjects/models/testVLLMOnMaaS.cy.ts`:
- Around line 136-138: The test currently queries the model row with
modelServingSection.findModelServerDeployedName(modelName) but doesn't assert
visibility; update the call to chain an explicit assertion so the element is
actually rendered (e.g.,
modelServingSection.findModelServerDeployedName(modelName).should('be.visible')),
referencing the existing findModelServerDeployedName helper and the modelName
variable.
- Line 52: The hardwareProfileResourceName is set directly from testData (unlike
projectName and modelName) causing collision risk; update the test to append a
unique suffix (e.g., a generated UUID or timestamp) to
hardwareProfileResourceName when assigning it from testData so it matches the
uniqueness strategy used for projectName and modelName, and then update
createCleanHardwareProfile (or make the YAML parameterizable) to accept and use
this custom resource name so creation and teardown target the same unique
resource. Ensure you reference and modify the hardwareProfileResourceName
assignment and the createCleanHardwareProfile call to pass the generated unique
name.
In `@packages/cypress/cypress/utils/oc_commands/llmInferenceServiceConfig.ts`:
- Around line 26-31: The value assigned to name from result.stdout.trim() is
used directly in deleteCommand which allows command injection; before building
deleteCommand sanitize or validate name the same way other input params are
handled (e.g., reuse the existing sanitization/validation helper or enforce a
strict regex like /^[A-Za-z0-9._-]+$/ and reject or throw on invalid values).
After validating/sanitizing name, construct deleteCommand and call
cy.exec(deleteCommand, { failOnNonZeroExit: false }) as before; ensure any
rejected or empty names are logged and skip execution to avoid injection.
- Around line 45-54: The function createCleanLLMInferenceServiceConfig currently
declares a void return but performs async Cypress operations; change its
signature to return the Chainable (e.g., Cypress.Chainable or Chainable<void>)
and return the chained call so callers can wait/handle errors: return
cleanupLLMInferenceServiceConfig(configName).then(() =>
createCustomResource(applicationNamespace, configYamlPath)); ensure
Cypress.Chainable is imported/available in the file and keep references to
cleanupLLMInferenceServiceConfig, createCustomResource, applicationNamespace and
configYamlPath intact.
---
Nitpick comments:
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.ts`:
- Around line 145-147: The test hardcodes the container image string in
checkLLMInferenceServiceConfigState, duplicating the YAML fixture value; update
the test to source that image from a single place by either defining a shared
constant (e.g., export IMAGE_VLLM_CPU and use it in this test and the fixture)
or by reading the fixture value at runtime and passing it into
checkLLMInferenceServiceConfigState(llmInferenceServiceConfigName, projectName,
{ containerImage }), ensuring references to the image are only changed in one
place and using the existing symbols llmInferenceServiceConfigName and
projectName to locate the call.
🪄 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
Run ID: 1b88e58a-6d23-413f-8f05-79d42fdd0b5b
📒 Files selected for processing (3)
packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.tspackages/cypress/cypress/utils/oc_commands/llmInferenceServiceConfig.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts (1)
67-78:⚠️ Potential issue | 🟠 MajorPass the hardware-profile name, not the YAML path, into setup.
createCleanHardwareProfile()inpackages/cypress/cypress/utils/oc_commands/hardwareProfiles.tsforwards its argument tocleanupHardwareProfiles(), which filters bymetadata.name. Passingresources/yaml/llmd-hardware-profile.yamlhere means the pre-clean step will never match the actual profile this test later selects and deletes viahardwareProfileResourceName. That leaves stale cluster state behind and makes retries/order depend on prior runs.As per coding guidelines, "Test data cleanup: delete resources created during tests in after() hooks." and "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/dataScienceProjects/models/testDeployLLMDServing.cy.ts` around lines 67 - 78, createCleanHardwareProfile is being called with the YAML path (hardwareProfileYamlPath) so its internal cleanup forwards that string to cleanupHardwareProfiles and never matches resources by metadata.name; change the setup call to pass the actual hardware profile name (use testData.hardwareProfileName or the same hardwareProfileResourceName variable that represents the resource name) instead of the YAML path so createCleanHardwareProfile(...) and cleanupHardwareProfiles(...) will correctly match and remove the created hardware profile.
♻️ Duplicate comments (1)
packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts (1)
80-87:⚠️ Potential issue | 🟠 MajorStill sharing the hardware profile across runs.
Setup creates the static profile from
resources/yaml/llmd-hardware-profile.yaml, while teardown deletes byhardwareProfileResourceName. Inpackages/cypress/cypress/utils/oc_commands/hardwareProfiles.ts,createCleanHardwareProfile()forwards its argument intocleanupHardwareProfiles(), so the pre-clean step is not targeting the same resource this spec later selects and removes. Retries or parallel runs can still recreate/delete each other’s cluster-scoped profile. Use one run-unique profile name and thread that exact identifier through creation, selection, and cleanup.As per coding guidelines, "Test data cleanup: delete resources created during tests in after() hooks." and "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/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts` around lines 80 - 87, The test is creating a static, shared hardware profile from resources/yaml/llmd-hardware-profile.yaml but deletes by the run-specific variable hardwareProfileResourceName, causing mismatched create/cleanup and cross-run interference; update the flow so createCleanHardwareProfile returns or accepts the same run-unique name used by the test (use hardwareProfileResourceName) and ensure cleanupHardwareProfiles is called with that exact identifier; specifically modify the calls around createCleanHardwareProfile and cleanupHardwareProfiles so the YAML creation is parameterized with hardwareProfileResourceName (or have createCleanHardwareProfile return the created name) and the after() hook calls cleanupHardwareProfiles(hardwareProfileResourceName) to guarantee per-run unique profile creation and removal.
🤖 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/modelServing.ts`:
- Around line 676-678: The selector for the serving runtime was changed to
`[data-label="Deployment resource"]` only in findServingRuntime(), but the other
helpers ModelServingGlobal.findServingRuntime() and
ModelServingRow.shouldHaveServingRuntime() still query `[data-label="Serving
runtime"]`, causing inconsistent access; update those helpers to use the same
`[data-label="Deployment resource"]` selector (or explicitly create a separate
helper if the two labels are meant to target different columns) so all
page-object access paths are aligned to the renamed column.
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts`:
- Around line 170-178: The test currently skips verifying that the deployed
LLMInferenceService reached Ready, allowing false positives; restore the
Ready-state gate by re-enabling the checkLLMInferenceServiceState call:
uncomment the cy.get<string>('@resourceName').then((resourceName) =>
checkLLMInferenceServiceState(resourceName, projectName, { checkReady: true }));
block after modelServingSection.findModelServerDeployedName(modelName), or if
RHOAIENG-56694 still blocks readiness checks, wrap the assertion in a
conditional skip (e.g., skip this scenario when the RHOAIENG-56694 flag is
active) so either the test asserts readiness via checkLLMInferenceServiceState
or the whole scenario is skipped.
- Around line 244-255: The test stops asserting after
modelServingWizard.findSubmitButton().click() — add a post-submit assertion to
verify the created LLMInferenceService or skip the spec until blocked checks are
restored: after the submit click call either
checkLLMInferenceServiceState(yamlEditorModelName, projectName, { checkReady:
true }) to validate the backend accepted the YAML, or use
modelServingGlobal.getDeploymentRow(yamlEditorModelName).findStatusLabel(ModelStateLabel.STARTED).should('exist')
to assert the deployment row appears; if those checks are currently blocked,
mark the test pending (skip) instead of leaving it as a no-op.
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts`:
- Around line 159-185: The test currently only calls
modelServingSection.findModelServerDeployedName(modelName) which merely verifies
a row rendered and not that the deployment is usable; restore and gate the
subsequent assertions on checkInferenceServiceState by calling
cy.get<string>('@resourceName').then(resourceName =>
checkInferenceServiceState(resourceName, projectName, { checkReady: true }))
before continuing to reload,
modelServingSection.findModelMetricsLink(modelName), attemptToClickTooltip(),
and modelExternalTester(modelName, projectName) — alternatively, if
RHOAIENG-56694 prevents readiness reliably, mark this spec pending/skipped until
that ticket is resolved.
---
Outside diff comments:
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.ts`:
- Around line 67-78: createCleanHardwareProfile is being called with the YAML
path (hardwareProfileYamlPath) so its internal cleanup forwards that string to
cleanupHardwareProfiles and never matches resources by metadata.name; change the
setup call to pass the actual hardware profile name (use
testData.hardwareProfileName or the same hardwareProfileResourceName variable
that represents the resource name) instead of the YAML path so
createCleanHardwareProfile(...) and cleanupHardwareProfiles(...) will correctly
match and remove the created hardware profile.
---
Duplicate comments:
In
`@packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.ts`:
- Around line 80-87: The test is creating a static, shared hardware profile from
resources/yaml/llmd-hardware-profile.yaml but deletes by the run-specific
variable hardwareProfileResourceName, causing mismatched create/cleanup and
cross-run interference; update the flow so createCleanHardwareProfile returns or
accepts the same run-unique name used by the test (use
hardwareProfileResourceName) and ensure cleanupHardwareProfiles is called with
that exact identifier; specifically modify the calls around
createCleanHardwareProfile and cleanupHardwareProfiles so the YAML creation is
parameterized with hardwareProfileResourceName (or have
createCleanHardwareProfile return the created name) and the after() hook calls
cleanupHardwareProfiles(hardwareProfileResourceName) to guarantee per-run unique
profile creation and removal.
🪄 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
Run ID: e0c71275-04e1-4398-91b1-66729c2d7b2c
📒 Files selected for processing (4)
packages/cypress/cypress/pages/modelServing.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testDeployLLMDServing.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testSingleModelAdminCreation.cy.tspackages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cypress/cypress/tests/e2e/dataScienceProjects/models/testVLLMOnMaaS.cy.ts
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
from
29a746c to
9a15cd4
Compare
|
/retest |
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
3 times, most recently
from
3a1c37c to
75073bd
Compare
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
from
75073bd to
954cc09
Compare
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
from
954cc09 to
6163052
Compare
emilys314
left a comment
emilys314
left a comment
There was a problem hiding this comment.
The test looks good though and looks functional
ashley-o0o
force-pushed
the
56046/e2e-vllm-on-maas
branch
from
0d92c31 to
da33dab
Compare
There was a problem hiding this comment.
Oh @ashley-o0o I didn't mean to say anything to revert this file back to predictive. I've definitely come around to having the legacy vLLM test that you had in the previous commit. It covers the generative legacy vLLM deployment path. Without it there is no coverage for it
Would you be able to re-add the changes you had before in this file (testSingleModelAdminCreation.cy.ts)?
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sridarna 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 |
|
/retest |
openshift-merge-bot
Bot
merged commit dd859a1
into
opendatahub-io:main
3 participants
Closes: RHOAIENG-56046
Description
e2e test for the vLLM on MaaS work.
Comments out areas that cause failure due to RHOAIENG-56694 bug causing models to be unable to start. Also made some adjustments to stop the models so we can properly edit/delete them in the test (tests were failing without).
How Has This Been Tested?
Ran locally against the nightly cluster
Test Impact
Updated tests for vLLM on MaaS work
Screenshots
/job/components/job/dashboard/job/dashboard-e2e-tests/272/Test_20Reports/
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