[RFR] Add SourcePlatform.edit() method #1709
Conversation
Signed-off-by: Nandini Chandra <[email protected]>
WalkthroughThis pull request extends the SourcePlatform model class with a new Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~18 minutes
Possibly related PRs
Suggested Reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
100-100: Remove or update the TODO comment.The TODO comment references adding an update method, which has now been implemented as the
edit()method below. Consider removing this comment or updating it to reflect remaining work (e.g., adding corresponding tests).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/models/administration/source-platform/source-platform.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
cypress/utils/utils.ts (3)
clickItemInKebabMenu(812-821)cancelForm(199-201)submitForm(194-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (2)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
109-118: LGTM! Method signature and edit trigger are well-structured.The method signature is clear with optional parameters, and the implementation correctly opens the platform view and triggers the edit action via the kebab menu.
120-121: LGTM! Cancel logic is correct.The cancel logic is consistent with the
create()method pattern and properly exits the operation when requested.
cypress/e2e/models/administration/source-platform/source-platform.ts
Outdated
Show resolved
Hide resolved
cypress/e2e/models/administration/source-platform/source-platform.ts
Outdated
Show resolved
Hide resolved
|
nachandr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/administration/source-platform/source-platform.ts(3 hunks)cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/models/administration/source-platform/source-platform.ts
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (3)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-140)cypress/utils/utils.ts (3)
exists(374-382)notExists(384-391)deleteByList(1163-1167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (4)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
19-19: LGTM!The addition of
deleteByListto the imports is correct and necessary for the cleanup of the credentials array.
25-25: LGTM!The array declaration is correct and properly typed.
61-61: LGTM!Correctly uses array indexing to access the first credential's name.
89-89: LGTM!The cleanup correctly uses
deleteByList()to delete all credentials from the array.
| for (let i = 0; i < 2; i++) { | ||
| const creds = new CredentialsSourceControlUsername( | ||
| data.getRandomCredentialsData( | ||
| CredentialType.sourceControl, | ||
| UserCredentials.usernamePassword, | ||
| false, | ||
| null, | ||
| null, | ||
| true | ||
| ) | ||
| ); | ||
| creds.name = "CF-CREDS" + data.getRandomNumber(1, 200); | ||
| creds.create(); | ||
| cloudFoundryCreds.push(creds); | ||
| } |
There was a problem hiding this comment.
Credential creation with skipped test creates unnecessary overhead.
The loop correctly creates two credentials, but since the test at line 56 is skipped, these credentials are created and then immediately deleted without being used. This adds unnecessary overhead to the test suite execution.
🤖 Prompt for AI Agents
In cypress/e2e/tests/administration/source-platform/crud.test.ts around lines 39
to 53, the loop creates two credentials that are immediately deleted because the
test at line 56 is skipped; remove this unnecessary setup from the global scope
and instead create the credentials only inside the test that uses them (or in a
before/beforeEach hook that is not executed for the skipped test). Update the
file to either (a) move the credential creation code into the specific test body
at line ~56, or (b) wrap it in a conditional/before hook so it only runs when
the dependent test will execute, ensuring credentials are not created when the
test is skipped.
| }); | ||
|
|
||
| it("Perform CRUD Tests on Cloud Foundry Source platform", function () { | ||
| it.skip("Perform CRUD Tests on Cloud Foundry Source platform", function () { |
There was a problem hiding this comment.
🧩 Analysis chain
Why is the CRUD test skipped?
The test that exercises the new edit() method is disabled with it.skip. Since this PR adds the edit() functionality, the test should be enabled to verify the implementation. If there's a specific reason for skipping (e.g., known issues, environment dependencies), please clarify or add a comment explaining why.
🏁 Script executed:
#!/bin/bash
# Inspect the test file around line 56 to understand context
cat cypress/e2e/tests/administration/source-platform/crud.test.ts | head -80 | tail -30Length of output: 942
🏁 Script executed:
#!/bin/bash
# Search for edit() method implementation related to SourcePlatform
rg -n "edit\s*\(" --type=ts --type=js -C3 | grep -i "source.*platform\|sourcePlatform" | head -50Length of output: 2081
🏁 Script executed:
#!/bin/bash
# Look for any TODO, FIXME, or skip-related comments around this test
rg -n -C5 "it\.skip.*CRUD\|Perform CRUD.*skip" cypress/e2e/tests/administration/source-platform/crud.test.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are other skipped tests in the same file to understand pattern
rg -n "it\.skip" cypress/e2e/tests/administration/source-platform/crud.test.tsLength of output: 83
🏁 Script executed:
#!/bin/bash
# Search for SourcePlatform class or edit method definition
ast-grep --pattern "class SourcePlatform { $$$ edit($_) { $$$ } $$$ }"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Broader search for edit method in SourcePlatform context
rg -n "edit\s*\(" --type=ts -A5 | grep -B10 -A5 "SourcePlatform"Length of output: 0
🏁 Script executed:
#!/bin/bash
# View the complete edit() method implementation
cat cypress/e2e/models/administration/source-platform/source-platform.ts | sed -n '110,150p'Length of output: 969
🏁 Script executed:
#!/bin/bash
# Check if edit() is just a stub or fully implemented
rg -n "edit\(" cypress/e2e/models/administration/source-platform/source-platform.ts -A30Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Look for any WIP, TODO, or FIXME markers in the SourcePlatform class
rg -n "WIP|TODO|FIXME|SKIP|skip" cypress/e2e/models/administration/source-platform/source-platform.tsLength of output: 57
Enable the skipped CRUD test now that edit() is fully implemented
The edit() method is complete (source-platform.ts lines 110-139) with full implementation for updating name, url, and credentials. The test properly exercises this functionality but remains disabled with it.skip. A TODO comment at line 101 ("Add update method and corresponding test") suggests this skip was intentional during development. Since the implementation is now complete, remove it.skip to enable test coverage.
🤖 Prompt for AI Agents
In cypress/e2e/tests/administration/source-platform/crud.test.ts around line 56,
the CRUD test for Cloud Foundry Source platform is still skipped using it.skip
even though source-platform.ts edit() is fully implemented; remove the skip to
enable the test by changing it.skip("Perform CRUD Tests on Cloud Foundry Source
platform", ...) to it("Perform CRUD Tests on Cloud Foundry Source platform",
...), ensure there are no remaining TODOs related to this test, run the test
suite and confirm it passes locally or update test data/setup if needed.
Signed-off-by: Nandini Chandra <[email protected]>
nachandr
force-pushed
the
source_platform_edit
branch
from
6f9009e to
4edf2f3
Compare
Signed-off-by: Nandini Chandra <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
101-101: Remove stale TODO comment.The TODO comment indicates that an update method should be added, but the
edit()method has now been implemented below (lines 110-140). This comment is now outdated and should be removed.Apply this diff:
- // TODO: Add update method and corresponding test delete(cancel = false): void {
♻️ Duplicate comments (3)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
110-140: Fix always-true condition causing unnecessary form submissions.The condition at line 136
if (updatedValues)is always true becauseupdatedValuesis a required parameter. This causes the form to be submitted even when no fields are actually updated (e.g., when passing an empty object{}or when all provided values match current values).This issue was previously flagged and remains unresolved.
Track whether any changes were made and only submit if changes occurred:
+ let hasChanges = false; if (updatedValues.name && updatedValues.name !== this.name) { this.fillName(updatedValues.name); this.name = updatedValues.name; + hasChanges = true; } if (updatedValues.url && updatedValues.url !== this.url) { this.fillUrl(updatedValues.url); this.url = updatedValues.url; + hasChanges = true; } if (updatedValues.credentials && updatedValues.credentials !== this.credentials) { this.selectCredentials(updatedValues.credentials); this.credentials = updatedValues.credentials; + hasChanges = true; } - if (updatedValues) { + if (hasChanges) { submitForm(); }cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
39-53: Credential creation with skipped test creates unnecessary overhead.The loop creates two credentials, but since the test at line 56 is skipped, these credentials are created and then immediately deleted without being used. This adds unnecessary overhead to the test suite execution.
Move the credential creation inside the test body or into a
beforeEachhook so credentials are only created when the test actually runs. Alternatively, if you plan to enable the test soon, you can leave this as-is.
56-56: Enable the skipped CRUD test now thatedit()is fully implemented.The
edit()method is complete and functional (source-platform.ts lines 110-140). Since the implementation is ready, this test should be enabled to verify the functionality. The stale TODO comment at line 101 of source-platform.ts also suggests this skip was temporary during development.Apply this diff to enable the test:
- it.skip("Perform CRUD Tests on Cloud Foundry Source platform", function () { + it("Perform CRUD Tests on Cloud Foundry Source platform", function () {
🧹 Nitpick comments (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (1)
72-82: Add comprehensive verification for edit operations.The variable declarations correctly use
const. However, the test only verifies that the platform exists by name after each edit operation. It doesn't verify that the URL or credentials were actually updated.Consider adding assertions to verify each field was updated:
const newName = "CF-" + "updatedName" + data.getRandomNumber(1, 200); platform.edit({ name: newName }); exists(newName); // TODO: Add verification that name is displayed correctly in the table/details const newURL = "https://api.bosh-updated-lite.com"; platform.edit({ url: newURL }); exists(newName); // TODO: Add verification that URL column shows newURL const newCreds = cloudFoundryCreds[1].name; platform.edit({ credentials: newCreds }); exists(newName); // TODO: Add verification that credentials column shows newCredsThis would require either checking the table row details or opening the edit form to verify the values match expectations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/administration/source-platform/source-platform.ts(3 hunks)cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts (3)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-141)cypress/utils/utils.ts (3)
exists(374-382)notExists(384-391)deleteByList(1163-1167)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
cypress/utils/utils.ts (3)
performRowActionByIcon(799-810)cancelForm(199-201)submitForm(194-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (5)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
22-22: LGTM!The
performRowActionByIconimport is necessary for the newedit()method implementation.
86-89: Verify the necessity of force-click.The button click now uses
{ force: true }, which bypasses Cypress's actionability checks. This can mask legitimate UI issues such as overlays, visibility problems, or disabled states.Confirm whether this force-click is necessary due to a known UI quirk, or if it's working around a problem that should be fixed in the application itself.
cypress/e2e/tests/administration/source-platform/crud.test.ts (3)
19-19: LGTM!The
deleteByListimport is necessary for cleaning up the array of credentials in theafterhook.
25-25: LGTM!Changing
cloudFoundryCredsto an array correctly supports the test's need for multiple credentials to test the credential update functionality.
89-89: LGTM!The use of
deleteByList(cloudFoundryCreds)correctly handles cleanup of the credentials array.
nachandr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
39-53: Avoid creating credentials when the test is skipped.This setup runs in before() even with it.skip, adding overhead. Move creation inside the test (or gate it), so it doesn’t execute when skipped.
- for (let i = 0; i < 2; i++) { - const creds = new CredentialsSourceControlUsername( - data.getRandomCredentialsData( - CredentialType.sourceControl, - UserCredentials.usernamePassword, - false, - null, - null, - true - ) - ); - creds.name = "CF-CREDS" + data.getRandomNumber(1, 200); - creds.create(); - cloudFoundryCreds.push(creds); - } + // Credentials created within the test to avoid unnecessary setup when skipped.Then, at the start of the test body, add:
+ for (let i = 0; i < 2; i++) { + const creds = new CredentialsSourceControlUsername( + data.getRandomCredentialsData( + CredentialType.sourceControl, + UserCredentials.usernamePassword, + false, + null, + null, + true + ) + ); + creds.name = "CF-CREDS" + data.getRandomNumber(1, 200); + creds.create(); + cloudFoundryCreds.push(creds); + }
56-58: Clarify/resolve skip; consider skipping the suite to avoid setup.If MTA-6241 is resolved, enable the test. If not, use describe.skip or env-gate to skip hooks too.
Proposed patterns:
- Enable: change it.skip(...) to it(...).
- Or skip suite: change describe([...], ...) to describe.skip([...], ...).
- Or gate: const testFn = Cypress.env("RUN_CF_CRUD") ? it : it.skip; testFn("...", ...);
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
110-146: Submit only when actual changes are made (fix can-submit logic).Object.keys(updatedValues).length > 0 triggers submit even when values equal current ones, leading to disabled submit and test failure/timeouts. Track hasChanges instead.
edit( updatedValues: Partial<{ name: string; url: string; credentials: string; }>, cancel = false ): void { SourcePlatform.open(); performRowActionByIcon(this.name, commonView.pencilAction); if (cancel) { cancelForm(); return; } - const { name, url, credentials } = updatedValues; + const { name, url, credentials } = updatedValues; + let hasChanges = false; - if (name && name !== this.name) { + if (name !== undefined && name !== this.name) { this.fillName(name); this.name = name; + hasChanges = true; } - if (url && url !== this.url) { + if (url !== undefined && url !== this.url) { this.fillUrl(url); this.url = url; + hasChanges = true; } - if (credentials && credentials !== this.credentials) { + if (credentials !== undefined && credentials !== this.credentials) { this.selectCredentials(credentials); this.credentials = credentials; + hasChanges = true; } - if (Object.keys(updatedValues).length > 0) { + if (hasChanges) { submitForm(); } }Also applies to: 126-141, 143-145
🧹 Nitpick comments (5)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
84-99: Avoid force-click on “Create new platform”.Element is already asserted visible and enabled; force: true can mask overlay/timing bugs. Click normally unless there’s a known UI quirk.
- .click({ force: true }); + .click();Also applies to: 89-89
126-141: Optional: Don’t block legitimate empty-string updates.Using truthy checks prevents setting fields to "" if ever allowed; prefer explicit undefined checks as in the diff above.
cypress/e2e/tests/administration/source-platform/crud.test.ts (3)
25-25: Prefer const and shorthand array type.The variable isn’t re-assigned; only mutated.
-let cloudFoundryCreds: Array<CredentialsSourceControlUsername> = []; +const cloudFoundryCreds: CredentialsSourceControlUsername[] = [];
73-83: Verify URL and credentials updates, not just name existence.Add targeted assertions after each edit to confirm the specific field changed in UI.
Example approaches:
- Re-open edit form and assert field values:
const newURL = "https://api.bosh-updated-lite.com"; platform.edit({ url: newURL }); - exists(newName); + // Re-open edit form (cancel after verification) + platform.edit({}, true); + cy.get("#url").should("have.value", newURL); + // Close without changes + const newCreds = cloudFoundryCreds[1].name; platform.edit({ credentials: newCreds }); - exists(newName); + platform.edit({}, true); + cy.get("#credentials-toggle-select-typeahead").should("contain", newCreds);
- Or assert table row cells for newURL/newCreds if selectors are available.
86-90: Optional: Make success-alert assertions less brittle.Exact message text can drift. Prefer contains/regex on the key part (e.g., “was successfully deleted”) or centralize expected strings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/administration/source-platform/source-platform.ts(3 hunks)cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-147)cypress/utils/utils.ts (4)
exists(374-382)checkSuccessAlert(326-331)notExists(384-391)deleteByList(1163-1167)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
cypress/utils/utils.ts (3)
performRowActionByIcon(799-810)cancelForm(199-201)submitForm(194-197)cypress/e2e/views/source-platform.view.ts (3)
name(16-16)url(18-18)credentials(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (3)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
22-22: Import looks good.performRowActionByIcon is used below; import is correct.
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
19-19: Import changes look good.checkSuccessAlert and deleteByList are used correctly.
95-95: Cleanup via deleteByList is fine.No-op on empty arrays; safe if test remains skipped.
nachandr
force-pushed
the
source_platform_edit
branch
from
bd5f550 to
33fbfb0
Compare
nachandr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
cypress/e2e/tests/administration/source-platform/crud.test.ts (3)
56-57: Test remains skipped with documented reason.The TODO comment now provides context (MTA-6241) for why the test is skipped, which addresses the concern about lack of explanation. However, the test still cannot verify the
edit()functionality until the infrastructure issue is resolved.
39-53: Credentials created but not used due to skipped test.The loop creates two credentials, but since the test at line 56 is skipped, these are created and then immediately deleted in the
afterhook without being used. This adds unnecessary overhead to the test suite.Consider moving this credential creation inside the test body or into a
beforeEachhook that only runs when the test is active.
73-83: Insufficient verification after edit operations.After editing the URL (line 78) and credentials (line 82), the test only verifies that the platform name exists but doesn't confirm that the URL or credentials were actually updated. This could allow bugs in the edit functionality to go undetected.
Consider adding assertions that verify the specific field changes, such as checking the platform details view or table cells to confirm the URL and credentials reflect the new values.
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
110-146: Form submits even when no fields are actually changed.The condition at line 143 checks whether
updatedValuescontains any keys, but doesn't verify that any fields were actually modified. For example, callingplatform.edit({ name: platform.name })will submit the form even though the name hasn't changed, because lines 128-130 will skip the update (name equals current name) but line 143 will still trigger submission (object has one key).Apply this diff to track actual changes:
+ let hasChanges = false; + if (name && name !== this.name) { this.fillName(name); this.name = name; + hasChanges = true; } if (url && url !== this.url) { this.fillUrl(url); this.url = url; + hasChanges = true; } if (credentials && credentials !== this.credentials) { this.selectCredentials(credentials); this.credentials = credentials; + hasChanges = true; } - if (Object.keys(updatedValues).length > 0) { + if (hasChanges) { submitForm(); }
🧹 Nitpick comments (1)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
101-101: Remove outdated TODO comment.The TODO mentions adding an update method, but the
edit()method below (lines 110-146) implements this functionality. Please remove this comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/administration/source-platform/source-platform.ts(3 hunks)cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
cypress/utils/utils.ts (3)
performRowActionByIcon(799-810)cancelForm(199-201)submitForm(194-197)cypress/e2e/views/source-platform.view.ts (3)
name(16-16)url(18-18)credentials(19-19)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-147)cypress/utils/utils.ts (4)
exists(374-382)checkSuccessAlert(326-331)notExists(384-391)deleteByList(1163-1167)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (4)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
22-22: Good addition to support the edit workflow.The import of
performRowActionByIconis necessary for the newedit()method and follows the existing pattern for row actions.
86-86: Timeout optimization looks reasonable.The reduced timeout (20s → 8s) is appropriate for a button that should be quickly available after page load.
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
19-19: Good refactoring to support multiple credentials.The addition of
deleteByListand the change to an array-basedcloudFoundryCredsproperly support the new test structure that creates and uses two credentials.Also applies to: 25-25
85-91: Deletion verification and cleanup look good.The success alert check after deletion follows the same pattern as the creation alert, and the use of
deleteByListfor cleaning up the credentials array is appropriate.Also applies to: 95-95
Signed-off-by: Nandini Chandra <[email protected]>
nachandr
force-pushed
the
source_platform_edit
branch
from
33fbfb0 to
ee16a7b
Compare
Signed-off-by: Nandini Chandra <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cypress/e2e/models/administration/source-platform/source-platform.ts (1)
109-145: Refine submit logic to track actual field changes.The current logic submits the form whenever
updatedValueshas any keys, even if no fields were actually modified. For example, callingedit({ name: this.name })would submit the form despite no change occurring.Track whether changes were made:
edit( updatedValues: Partial<{ name: string; url: string; credentials: string; }>, cancel = false ): void { SourcePlatform.open(); performRowActionByIcon(this.name, commonView.pencilAction); if (cancel) { cancelForm(); return; } const { name, url, credentials } = updatedValues; + let hasChanges = false; if (name && name !== this.name) { this.fillName(name); this.name = name; + hasChanges = true; } if (url && url !== this.url) { this.fillUrl(url); this.url = url; + hasChanges = true; } if (credentials && credentials !== this.credentials) { this.selectCredentials(credentials); this.credentials = credentials; + hasChanges = true; } - if (Object.keys(updatedValues).length > 0) { + if (hasChanges) { submitForm(); } }cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
39-53: Move credential creation inside the test or conditional hook.Since the test at line 56 is skipped, these two credentials are created during setup and immediately deleted in teardown without being used. This adds unnecessary overhead to the test suite execution.
Move the credential creation into the test body or a
beforeEachhook that only runs when the test executes:- before("Login", function () { + before("Login and setup", function () { if ( !Cypress.env("cloudfoundry_user") || !Cypress.env("cloudfoundry_password") || !Cypress.env("cloudfoundry_url") ) { throw new Error(`One or more required Cloud Foundry env variables are missing in cypress.config.ts : \ncloudfoundry_user\ncloudfoundry_password\ncloudfoundry_url`); } login(); cy.visit("/"); - for (let i = 0; i < 2; i++) { - const creds = new CredentialsSourceControlUsername( - data.getRandomCredentialsData( - CredentialType.sourceControl, - UserCredentials.usernamePassword, - false, - null, - null, - true - ) - ); - creds.name = `CF-CREDS-${data.getRandomNumber(1, 500)}`; - creds.create(); - cloudFoundryCreds.push(creds); - } }); it.skip("Perform CRUD Tests on Cloud Foundry Source platform", function () { // TODO : Unskip tests once Infra ticket MTA-6241 is resolved + for (let i = 0; i < 2; i++) { + const creds = new CredentialsSourceControlUsername( + data.getRandomCredentialsData( + CredentialType.sourceControl, + UserCredentials.usernamePassword, + false, + null, + null, + true + ) + ); + creds.name = `CF-CREDS-${data.getRandomNumber(1, 500)}`; + creds.create(); + cloudFoundryCreds.push(creds); + } + const platform = new SourcePlatform(
73-83: Strengthen edit verification assertions.The test only verifies that the platform name exists after each edit operation. It doesn't confirm that the URL or credentials were actually updated in the UI or backend.
Add assertions to verify each edited field. For example, after editing the URL, query the platform details or table to confirm the new URL is displayed:
const newName = `CF-updatedName-${data.getRandomNumber(1, 500)}`; platform.edit({ name: newName }); exists(newName); const newURL = "https://api.bosh-updated-lite.com"; platform.edit({ url: newURL }); exists(newName); + // TODO: Add assertion to verify newURL is displayed in the platform row or details const newCreds = cloudFoundryCreds[1].name; platform.edit({ credentials: newCreds }); exists(newName); + // TODO: Add assertion to verify newCreds is displayed in the platform row or details
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/models/administration/source-platform/source-platform.ts(3 hunks)cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cypress/e2e/models/administration/source-platform/source-platform.ts (3)
cypress/e2e/types/constants.ts (1)
cancel(74-74)cypress/utils/utils.ts (3)
performRowActionByIcon(799-810)cancelForm(199-201)submitForm(194-197)cypress/e2e/views/source-platform.view.ts (3)
name(16-16)url(18-18)credentials(19-19)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-146)cypress/utils/utils.ts (4)
exists(374-382)checkSuccessAlert(326-331)notExists(384-391)deleteByList(1163-1167)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (8)
cypress/e2e/models/administration/source-platform/source-platform.ts (2)
22-22: LGTM!The
performRowActionByIconimport is correctly added to support the newedit()method implementation.
86-86: Verify the reduced timeout doesn't cause flakiness.The timeout was reduced from 20000ms to 8000ms. Ensure this is sufficient for the "Create new platform" button to appear in all test environments.
cypress/e2e/tests/administration/source-platform/crud.test.ts (6)
19-19: LGTM!The additional imports
checkSuccessAlertanddeleteByListare appropriately added for the enhanced test assertions and cleanup.
25-25: LGTM!Refactoring to an array of credentials enables testing multiple credential updates.
50-50: LGTM!Using template literals with a broader range (1-500) reduces naming collisions.
56-57: Skipped test prevents validation of the new edit() functionality.While the TODO comment explains the infrastructure dependency (MTA-6241), this PR adds the
edit()method but doesn't enable test coverage. Consider adding a simpler unit-level test or integration test with mock data that can run without the Cloud Foundry infrastructure.
86-90: LGTM!Verifying the success alert after deletion ensures the operation completed as expected.
95-95: LGTM!Using
deleteByListproperly cleans up all created credentials in a single call.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
39-53: Credential creation overhead persists with skipped test.This loop creates two credentials that are never used because the test at line 56 remains skipped. While the TODO comment at line 57 indicates this skip is temporary pending resolution of ticket MTA-6241, consider moving credential creation into a
beforeEachhook or directly into the test body to avoid unnecessary setup execution when the test is disabled.
56-57: Test remains disabled pending infrastructure resolution.The TODO comment now explains the skip is due to infrastructure ticket MTA-6241. While this provides necessary context, the
edit()method implementation remains untested in the CI pipeline. Consider setting up a follow-up mechanism to enable this test once the infrastructure issue is resolved.
🧹 Nitpick comments (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (1)
73-83: Previous feedback addressed; consider verifying edited values in UI.The code now correctly uses
constdeclarations and proper naming format with backticks and hyphens, addressing earlier review feedback.Each edit operation is followed by an
exists(newName)check, which confirms the platform row is present but doesn't verify that the specific field (URL or credentials) was actually updated in the UI. For more robust verification, consider adding assertions that check the updated values are displayed correctly in the platform table or details view.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-146)cypress/utils/utils.ts (4)
exists(374-382)checkSuccessAlert(326-331)notExists(384-391)deleteByList(1163-1167)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (5)
cypress/e2e/tests/administration/source-platform/crud.test.ts (5)
19-19: LGTM! Imports are correctly used.The new imports (
checkSuccessAlertanddeleteByList) are appropriately utilized in the test body for verifying success alerts and cleaning up the credential array.
25-25: LGTM! Array type correctly declared.The change from a single credential to an array is appropriate given the test now creates and uses multiple credentials.
58-63: LGTM! Platform creation correctly updated.The platform construction properly uses
cloudFoundryCreds[0].namefor array access, and the expanded random range (1-500) reduces the likelihood of name collisions.
85-90: LGTM! Deletion verification improved.Adding the success alert check after deletion improves test robustness by explicitly verifying the deletion operation completed successfully with the expected message.
94-96: LGTM! Cleanup correctly handles credential array.Using
deleteByList()is the appropriate approach for cleaning up the array of credentials, as it automatically iterates and deletes each element.
|
|
||
| const newCreds = cloudFoundryCreds[1].name; | ||
| platform.edit({ credentials: newCreds }); | ||
| exists(newName); |
There was a problem hiding this comment.
This is not checking if the URL is updated .
nachandr
force-pushed
the
source_platform_edit
branch
from
8c90f62 to
fe31a40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
64-65: Test remains skipped pending infrastructure fix.The test is appropriately marked with
it.skipand includes a TODO referencing the blocking infra ticket (MTA-6241). However, since theedit()implementation is complete, consider adding a timeline or link to track when this can be enabled.Would you like me to help track this issue or add a reminder to enable the test once MTA-6241 is resolved?
47-61: Credentials still created unnecessarily when test is skipped.The credential creation loop runs in the
beforehook even though the test at line 64 is skipped. This adds unnecessary overhead (~2-4 seconds) to the test suite. Move credential creation inside the test body or use abeforeEachhook that only runs when the test executes.Apply this diff to move setup into the test:
- before("Login", function () { + before("Login", function () { if ( !Cypress.env("cloudfoundry_user") || !Cypress.env("cloudfoundry_password") || !Cypress.env("cloudfoundry_url") ) { throw new Error(`One or more required Cloud Foundry env variables are missing in cypress.config.ts : \ncloudfoundry_user\ncloudfoundry_password\ncloudfoundry_url`); } login(); cy.visit("/"); - for (let i = 0; i < 2; i++) { - const creds = new CredentialsSourceControlUsername( - data.getRandomCredentialsData( - CredentialType.sourceControl, - UserCredentials.usernamePassword, - false, - null, - null, - true - ) - ); - creds.name = `CF-CREDS-${data.getRandomNumber(1, 500)}`; - creds.create(); - cloudFoundryCreds.push(creds); - } }); it.skip("Perform CRUD Tests on Cloud Foundry Source platform", function () { // TODO : Unskip tests once Infra ticket MTA-6241 is resolved + for (let i = 0; i < 2; i++) { + const creds = new CredentialsSourceControlUsername( + data.getRandomCredentialsData( + CredentialType.sourceControl, + UserCredentials.usernamePassword, + false, + null, + null, + true + ) + ); + creds.name = `CF-CREDS-${data.getRandomNumber(1, 500)}`; + creds.create(); + cloudFoundryCreds.push(creds); + } + const platform = new SourcePlatform(
🧹 Nitpick comments (1)
cypress/utils/utils.ts (1)
148-150: Consider adding a timeout parameter for better reliability.The
verifyInputValuehelper doesn't specify a timeout on thecy.get()call. While Cypress applies a default timeout, adding an explicit timeout (consistent with other helpers likeinputTextat line 139) would improve reliability when the field hasn't rendered yet.Apply this diff to add a timeout:
-export function verifyInputValue(fieldID: string, expectedValue: string): void { - cy.get(fieldID).invoke("val").should("equal", expectedValue); +export function verifyInputValue(fieldID: string, expectedValue: string): void { + cy.get(fieldID, { timeout: 2 * SEC }).invoke("val").should("equal", expectedValue); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)cypress/utils/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-146)cypress/utils/utils.ts (5)
exists(378-386)verifyInputValue(148-150)checkSuccessAlert(330-335)notExists(388-395)deleteByList(1167-1171)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (2)
cypress/e2e/tests/administration/source-platform/crud.test.ts (2)
19-31: LGTM!The import additions are appropriate for the new test functionality—
deleteByListfor cleanup,verifyInputValuefor assertions, and the selectors for field identification.
102-104: LGTM!The cleanup correctly uses
deleteByListto handle the array of credentials.
nachandr
force-pushed
the
source_platform_edit
branch
from
fe31a40 to
0c4e0d1
Compare
Signed-off-by: Nandini Chandra <[email protected]>
nachandr
force-pushed
the
source_platform_edit
branch
from
0c4e0d1 to
9857be2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (1)
40-54: Credential creation inbeforehook runs even when test is skipped.The loop correctly creates two credentials, but they are instantiated in the
beforehook which executes regardless of theit.skipat line 57. This creates unnecessary overhead—credentials are created and immediately deleted without being used.However, given the TODO comment at line 58 explaining the skip is temporary (pending infra ticket MTA-6241), this overhead is acceptable for now.
Minor naming note: Line 51 uses a hyphen in
CF-CREDS-${...}, whereas a past review suggested underscore formatCF-CREDS_${...}. Consider aligning with that convention for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cypress/e2e/tests/administration/source-platform/crud.test.ts (4)
cypress/e2e/models/administration/credentials/credentialsSourceControlUsername.ts (1)
CredentialsSourceControlUsername(15-113)cypress/e2e/models/administration/source-platform/source-platform.ts (1)
SourcePlatform(33-146)cypress/utils/utils.ts (4)
exists(374-382)checkSuccessAlert(326-331)notExists(384-391)deleteByList(1163-1167)cypress/e2e/views/common.view.ts (1)
successAlertMessage(31-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-main / e2e-ui-integration-tests
🔇 Additional comments (6)
cypress/e2e/tests/administration/source-platform/crud.test.ts (6)
19-19: LGTM! Import additions align with new functionality.The added imports (
checkSuccessAlert,deleteByList, andselectors) are used appropriately throughout the test file.Also applies to: 24-24
26-26: LGTM! Array initialization supports multiple credentials.The change from a single credential object to an array correctly supports the test's need for two credentials (one for initial platform creation, another for credential update testing).
57-58: LGTM! Test skip is justified with clear TODO.The test is appropriately skipped with a TODO comment that references the blocking infrastructure ticket (MTA-6241). This provides clear context for when the test should be re-enabled.
59-64: LGTM! Platform creation correctly uses array-indexed credentials.The changes properly reference
cloudFoundryCreds[0].nameto match the new array structure, and the expanded random range (1-500) reduces the likelihood of name collisions.
66-72: LGTM! Platform creation with proper verification.The platform creation flow correctly verifies both the success alert message and the platform's existence in the table.
86-96: LGTM! Deletion verification and cleanup properly implemented.Good improvements:
- Lines 87-91 add success alert verification after deletion, consistent with the create operation
- Line 96 correctly uses
deleteByList()to clean up the credentials array
| const newName = `CF-updatedName-${data.getRandomNumber(1, 500)}`; | ||
| platform.edit({ name: newName }); | ||
| exists(newName); | ||
|
|
||
| const newURL = "https://api.bosh-updated-lite.com"; | ||
| platform.edit({ url: newURL }); | ||
| cy.get(selectors.url).should("have.value", newURL); | ||
|
|
||
| const newCreds = cloudFoundryCreds[1].name; | ||
| platform.edit({ credentials: newCreds }); | ||
| cy.get(selectors.credentials).should("have.value", newCreds); |
There was a problem hiding this comment.
Critical: Form field verifications at lines 80 and 84 will fail after form closes.
Lines 80 and 84 attempt to verify selectors.url and selectors.credentials values after calling platform.edit(). However, the edit() method calls submitForm() (source-platform.ts line 138), which closes the edit form and returns to the list page. At that point, the form input fields referenced by these selectors are no longer visible in the DOM, causing these assertions to fail.
Since the test is currently skipped (line 57), this issue hasn't been caught yet but will cause failures when the test is re-enabled.
Recommended fix:
Option 1 - Verify via success alerts (recommended):
const newURL = "https://api.bosh-updated-lite.com";
platform.edit({ url: newURL });
-cy.get(selectors.url).should("have.value", newURL);
+checkSuccessAlert(
+ successAlertMessage,
+ `Success alert:Source platform ${platform.name} was successfully updated.`,
+ true
+);
const newCreds = cloudFoundryCreds[1].name;
platform.edit({ credentials: newCreds });
-cy.get(selectors.credentials).should("have.value", newCreds);
+checkSuccessAlert(
+ successAlertMessage,
+ `Success alert:Source platform ${platform.name} was successfully updated.`,
+ true
+);Option 2 - Re-open edit form to verify, then cancel:
const newURL = "https://api.bosh-updated-lite.com";
platform.edit({ url: newURL });
+SourcePlatform.open();
+performRowActionByIcon(platform.name, commonView.pencilAction);
cy.get(selectors.url).should("have.value", newURL);
+cancelForm();Note: You'll need to import performRowActionByIcon and cancelForm for option 2.
📝 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.
| const newName = `CF-updatedName-${data.getRandomNumber(1, 500)}`; | |
| platform.edit({ name: newName }); | |
| exists(newName); | |
| const newURL = "https://api.bosh-updated-lite.com"; | |
| platform.edit({ url: newURL }); | |
| cy.get(selectors.url).should("have.value", newURL); | |
| const newCreds = cloudFoundryCreds[1].name; | |
| platform.edit({ credentials: newCreds }); | |
| cy.get(selectors.credentials).should("have.value", newCreds); | |
| const newName = `CF-updatedName-${data.getRandomNumber(1, 500)}`; | |
| platform.edit({ name: newName }); | |
| exists(newName); | |
| const newURL = "https://api.bosh-updated-lite.com"; | |
| platform.edit({ url: newURL }); | |
| checkSuccessAlert( | |
| successAlertMessage, | |
| `Success alert:Source platform ${platform.name} was successfully updated.`, | |
| true | |
| ); | |
| const newCreds = cloudFoundryCreds[1].name; | |
| platform.edit({ credentials: newCreds }); | |
| checkSuccessAlert( | |
| successAlertMessage, | |
| `Success alert:Source platform ${platform.name} was successfully updated.`, | |
| true | |
| ); |
🤖 Prompt for AI Agents
In cypress/e2e/tests/administration/source-platform/crud.test.ts around lines 74
to 84, the test asserts form input values after calling platform.edit(), but
edit() submits and closes the form (see source-platform.ts line 138) so those
selectors are removed and assertions will fail; fix by either (preferred)
replacing the direct field assertions with checks against the post-submit
success alert/message (assert the alert contains the new URL/credentials) or
(alternative) reopen the edit form for the same row (use performRowActionByIcon
to re-open), assert the input values, then call cancelForm to close without
resubmitting (ensure you import performRowActionByIcon and cancelForm).
2 participants
Summary by CodeRabbit
Release Notes