test: adapt UI e2e tests to prod environment#1271
test: adapt UI e2e tests to prod environment#1271rsoaresd merged 13 commits intocodeready-toolchain:masterfrom
Conversation
|
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:
WalkthroughAdds production-capable E2E test support: externalizes environment/kubeconfig, adds prod make targets and README docs, adapts tests and login/setup for prod behavior, introduces ksctl-based user-signup management, and refactors test artifact/video handling. Changes
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant Ksctl as ksctl CLI
participant Prod as Prod Environment
participant Page as Browser Page
rect rgba(76, 175, 80, 0.5)
Note over Test,Page: Production flow (ProdEnv)
Test->>Ksctl: ksctl get usersignup <username>
Ksctl->>Prod: request UserSignup (yaml)
Prod-->>Ksctl: UserSignup (yaml)
Ksctl-->>Test: parsed UserSignup
Test->>Page: Navigate to https://sandbox.redhat.com/
Test->>Page: Fill creds & submit (Next / Log in)
Test->>Page: Detect /oauth/authorize popup
Page-->>Test: redirect to project page & welcome text
Test->>Ksctl: ksctl gdpr-delete <username> (stdin "y")
Ksctl->>Ksctl: poll get usersignup until NotFound
Ksctl-->>Test: deletion confirmed
end
rect rgba(33, 150, 243, 0.5)
Note over Test,Page: Non-production flow (Test/Dev)
Test->>Page: host-based signup cleanup
Test->>Page: wait for image/heading/list assertions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
testsupport/devsandbox-dashboard/trace.go (1)
97-125: MakemaskUsername()idempotent per browser context.This helper installs a new init script every time it is called.
LoginPage.Navigate()andClickAndWaitForPopup()now both invoke it on the same context, so each popup open registers another copy of the script and anotherMutationObserverfor future pages. Add a guard here so repeated calls become a no-op.Possible minimal guard inside the injected script
func maskUsername(t *testing.T, page playwright.Page) { err := page.Context().AddInitScript(playwright.Script{ Content: playwright.String(` + if (window.__usernameMaskInitialized) { + return; + } + window.__usernameMaskInitialized = true; + const applyBlur = () => { document.querySelectorAll('input[name="username"], span.co-username, span[data-test="username"], [data-test="username"]').forEach(el => { el.style.filter = 'blur(5px)'; el.style.userSelect = 'none'; }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/devsandbox-dashboard/trace.go` around lines 97 - 125, maskUsername registers the same init script each call causing multiple MutationObservers; modify the injected script (the one passed to page.Context().AddInitScript in maskUsername) to be idempotent by checking/setting a per-context flag (e.g. window.__maskUsernameInstalled) at the top: if the flag is already true, return immediately; otherwise set it true and then run applyBlur + create the MutationObserver. Keep the rest of the applyBlur/observer logic intact so repeated calls become no-ops for that browser context.testsupport/devsandbox-dashboard/usersignup.go (1)
66-99: Consider extracting repeated command arguments to constants.SonarCloud flagged the
"--config"literal being duplicated 3 times (lines 48, 70, 81). You could extract common ksctl command arguments to reduce duplication.♻️ Proposed refactor
+const ( + ksctlConfigFlag = "--config" + ksctlHostTarget = "-t" +) + func GetUserSignupThroughKsctl(t *testing.T, username string) *toolchainv1alpha1.UserSignup { t.Logf("Getting UserSignup through ksctl") // `#nosec` G204 -- username is from test config, not user input - cmd := exec.Command("ksctl", "get", "usersignup", username, "--config", viper.GetString("KUBECONFIG"), "-t", "host", "-o", "yaml") + cmd := exec.Command("ksctl", "get", "usersignup", username, ksctlConfigFlag, viper.GetString("KUBECONFIG"), ksctlHostTarget, "host", "-o", "yaml")Apply similarly to lines 70 and 81.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/devsandbox-dashboard/usersignup.go` around lines 66 - 99, The function DeleteUserSignupThroughKsctl repeats the same ksctl arguments (the "--config" flag and viper.GetString("KUBECONFIG")) in multiple exec.Command calls; extract the common args into a single reusable variable or helper (e.g., ksctlConfigArg := []string{"--config", viper.GetString("KUBECONFIG")} or a buildKsctlCmd helper) and use it when constructing exec.Command for the initial "ksctl gdpr-delete" call and the subsequent "ksctl get usersignup" calls to remove duplication and make intent clearer while preserving existing command composition and stdin behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/devsandbox-dashboard/setup.go`:
- Around line 67-71: The current gate using ARTIFACT_DIR prevents trace(t,
context, testName) from running in CI and removes Playwright traces; instead
remove the ARTIFACT_DIR check and always call trace(...) (so traces continue to
be routed by getTraceDirectory()), or if you need to disable tracing only for
production, gate on env == ProdEnv or a dedicated opt-out flag (e.g.
DISABLE_TRACING) rather than ARTIFACT_DIR; update
testsupport/devsandbox-dashboard/setup.go to call trace(t, context, testName)
unconditionally or replace the ARTIFACT_DIR condition with the appropriate
ProdEnv/opt-out check.
---
Nitpick comments:
In `@testsupport/devsandbox-dashboard/trace.go`:
- Around line 97-125: maskUsername registers the same init script each call
causing multiple MutationObservers; modify the injected script (the one passed
to page.Context().AddInitScript in maskUsername) to be idempotent by
checking/setting a per-context flag (e.g. window.__maskUsernameInstalled) at the
top: if the flag is already true, return immediately; otherwise set it true and
then run applyBlur + create the MutationObserver. Keep the rest of the
applyBlur/observer logic intact so repeated calls become no-ops for that browser
context.
In `@testsupport/devsandbox-dashboard/usersignup.go`:
- Around line 66-99: The function DeleteUserSignupThroughKsctl repeats the same
ksctl arguments (the "--config" flag and viper.GetString("KUBECONFIG")) in
multiple exec.Command calls; extract the common args into a single reusable
variable or helper (e.g., ksctlConfigArg := []string{"--config",
viper.GetString("KUBECONFIG")} or a buildKsctlCmd helper) and use it when
constructing exec.Command for the initial "ksctl gdpr-delete" call and the
subsequent "ksctl get usersignup" calls to remove duplication and make intent
clearer while preserving existing command composition and stdin behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40b3f0e3-cf7f-4d4d-b833-c27d59928c58
📒 Files selected for processing (9)
deploy/devsandbox-dashboard/ui-e2e-tests/.envmake/devsandbox-dashboard.mktest/e2e/devsandbox-dashboard/README.mdtest/e2e/devsandbox-dashboard/fresh_signup_test.gotestsupport/devsandbox-dashboard/login.gotestsupport/devsandbox-dashboard/popup.gotestsupport/devsandbox-dashboard/setup.gotestsupport/devsandbox-dashboard/trace.gotestsupport/devsandbox-dashboard/usersignup.go
| // save trace only if not running in CI | ||
| // we do not want to expose sensitive information in CI | ||
| if artifactDir := os.Getenv("ARTIFACT_DIR"); artifactDir == "" { // not CI environment | ||
| trace(t, context, testName) | ||
| } |
There was a problem hiding this comment.
Don't disable tracing for every CI run.
ARTIFACT_DIR is already how getTraceDirectory() routes trace artifacts into CI storage. Gating trace() on that variable removes Playwright traces from all CI jobs, including the existing ui-e2e-tests path, so failures lose the most useful debugging artifact. If the concern is prod secrecy, gate on env == ProdEnv or a dedicated opt-out flag instead of CI detection.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 69-69: Remove this unnecessary variable declaration and use the expression directly in the condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testsupport/devsandbox-dashboard/setup.go` around lines 67 - 71, The current
gate using ARTIFACT_DIR prevents trace(t, context, testName) from running in CI
and removes Playwright traces; instead remove the ARTIFACT_DIR check and always
call trace(...) (so traces continue to be routed by getTraceDirectory()), or if
you need to disable tracing only for production, gate on env == ProdEnv or a
dedicated opt-out flag (e.g. DISABLE_TRACING) rather than ARTIFACT_DIR; update
testsupport/devsandbox-dashboard/setup.go to call trace(t, context, testName)
unconditionally or replace the ARTIFACT_DIR condition with the appropriate
ProdEnv/opt-out check.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
testsupport/devsandbox-dashboard/setup.go (1)
67-71:⚠️ Potential issue | 🟠 MajorDon't key tracing off
ARTIFACT_DIR.
getTraceDirectory()already routes CI traces intoARTIFACT_DIR. This guard disables Playwright tracing in every CI job, so failedui-e2e-testsruns lose the main debugging artifact. If only prod runs need different handling, gate onenv == ProdEnvor a dedicated opt-out flag instead of CI detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/devsandbox-dashboard/setup.go` around lines 67 - 71, The current guard that skips trace(t, context, testName) based on os.Getenv("ARTIFACT_DIR") prevents CI jobs from saving Playwright traces; instead remove that ARTIFACT_DIR check and either always call trace(...) or gate it on the production-only condition (compare the current environment to ProdEnv) or a dedicated opt-out flag (e.g. DISABLE_TRACING) so CI traces still get written via getTraceDirectory(); update the logic around trace(...) and any callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/devsandbox-dashboard/usersignup.go`:
- Around line 54-60: GetUserSignupThroughKsctl() and the delete-poll block
currently treat ksctl "not found" responses differently and swallow other
errors; update both places so they consistently detect NotFound
(case-insensitive match for "not found" and the exact "Error from server
(NotFound)") and treat that as the missing-resource success case, but for any
other non-NotFound error return the error immediately (fail fast) instead of
returning nil/false and continuing to poll; apply the same change to the other
occurrence referenced in the comment (the delete poll block around lines 87-100)
so both code paths behave the same.
---
Duplicate comments:
In `@testsupport/devsandbox-dashboard/setup.go`:
- Around line 67-71: The current guard that skips trace(t, context, testName)
based on os.Getenv("ARTIFACT_DIR") prevents CI jobs from saving Playwright
traces; instead remove that ARTIFACT_DIR check and either always call trace(...)
or gate it on the production-only condition (compare the current environment to
ProdEnv) or a dedicated opt-out flag (e.g. DISABLE_TRACING) so CI traces still
get written via getTraceDirectory(); update the logic around trace(...) and any
callers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4b2b2ec-2c4c-4f02-9228-10653211e4ac
📒 Files selected for processing (3)
test/e2e/devsandbox-dashboard/fresh_signup_test.gotestsupport/devsandbox-dashboard/setup.gotestsupport/devsandbox-dashboard/usersignup.go
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Great job 👏
I have only few minor comments/questions.
| return strings.Replace(targetPath, "popup", fmt.Sprintf("popup-%s", uuid), 1) | ||
| } | ||
|
|
||
| func maskUsername(t *testing.T, page playwright.Page) { |
There was a problem hiding this comment.
I think the extra caution of masking also the username doesn't harm. Hopefully this doesn't cause flakyness .
I'm thinking about the case where the SSO login page might change they CSS selectors.
Let's keep it as is, but if we see it breaks often I would be in favor of removing it since sandbox is free to use and anyone can signup easly for a new account.
There was a problem hiding this comment.
stupid question, what is this masking used for? What is its purpose?
There was a problem hiding this comment.
I spoke with Matous in our 1on1. Let's drop the masking of username since it is not necessary. Sorry for complicating and for the noise
| require.NoError(t, err) | ||
| } | ||
| case sandboxui.ProdEnv: | ||
| userSignup := sandboxui.GetUserSignupThroughKsctl(t, username) |
There was a problem hiding this comment.
do we actually need this? creating UserSignup should be a deterministic process, so there should be always the same UserSignup for the given username.
There was a problem hiding this comment.
I thought about calling GetUserSignupThroughKsctl here in order to just call the delete func if usersignup exists
| userSignup := sandboxui.GetUserSignupThroughKsctl(t, username) | ||
| if userSignup != nil { | ||
| // delete user signup | ||
| sandboxui.DeleteUserSignupThroughKsctl(t, username) |
There was a problem hiding this comment.
as we already discussed in our 1-1, it could be simplified by only deleting the UserSignup directly. but using ksctl is OK too 👍 feel free to keep it as it is
|
@MatousJobanek @mfrancisc thank you so much for your inputs! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/devsandbox-dashboard/fresh_signup_test.go (1)
43-63:⚠️ Potential issue | 🟠 MajorHandle
DevEnvexplicitly instead of silently skipping cleanup.
testsupport/devsandbox-dashboard/setup.gostill exposesdevas a supported environment, but both switches fall through for it. Indev, this test now skips the pre-run delete and never registers teardown, so reruns can inherit staleUserSignupstate. Either routedevto the correct strategy or fail fast on unsupported values.Minimal guardrail
switch env { case sandboxui.TestEnv: ... case sandboxui.ProdEnv: ... +default: + require.Failf(t, "unsupported ENVIRONMENT", "unexpected ENVIRONMENT=%q", env) }Apply the same guard to both switches until
sandboxui.DevEnvis wired to the intended cleanup path.Also applies to: 136-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go` around lines 43 - 63, The switch in ensureNoUserSignup does not handle sandboxui.DevEnv and silently skips cleanup; update ensureNoUserSignup to explicitly handle sandboxui.DevEnv (either route it to the same cleanup flow as sandboxui.TestEnv by using testsupport.WaitForDeployments/hostAwait and sandboxui.WaitForUserSignup/DeleteUserSignup, or return an explicit failure for unsupported envs), and apply the same explicit handling to the other similar switch later in the file (the second occurrence around the teardown logic) so DevEnv does not fall through silently.
♻️ Duplicate comments (1)
testsupport/devsandbox-dashboard/usersignup.go (1)
54-60:⚠️ Potential issue | 🟠 MajorBroaden ksctl
NotFoundmatching beforerequire.NoError.
DeleteUserSignupThroughKsctl()now depends on this helper in its poll loop. Matching onlyError from server (NotFound)means a valid missing-resource response with different casing/wording turns a completed delete into a hard failure instead of the expectednilresult. Please verify this against the ksctl version used in CI.Possible fix
+func ksctlUserSignupNotFound(output []byte, err error) bool { + text := strings.ToLower(string(output)) + errText := "" + if err != nil { + errText = strings.ToLower(err.Error()) + } + + return strings.Contains(text, "not found") || + strings.Contains(text, "notfound") || + strings.Contains(errText, "not found") || + strings.Contains(errText, "notfound") +} + func GetUserSignupThroughKsctl(t *testing.T, username string) *toolchainv1alpha1.UserSignup { t.Logf("Getting UserSignup through ksctl") // `#nosec` G204 -- username is from test config, not user input cmd := exec.Command("ksctl", "get", "usersignup", username, configFlag, viper.GetString("KUBECONFIG"), "-t", "host", "-o", "yaml") output, err := cmd.CombinedOutput() - if err != nil { - // Check if it's a not found error - if strings.Contains(string(output), "Error from server (NotFound)") { - return nil - } + if err != nil && ksctlUserSignupNotFound(output, err) { + return nil } require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsupport/devsandbox-dashboard/usersignup.go` around lines 54 - 60, The current check only matches the exact substring "Error from server (NotFound)" and should be made more resilient: in the block inside DeleteUserSignupThroughKsctl()'s helper (the if err != nil { ... } section referencing output), replace the strict match with a case-insensitive/looser check (e.g., lowercase output and check for "notfound" or "not found", or a case-insensitive regex like `(?i)not\s*found`) so variants of ksctl/ kubectl missing-resource messages are treated as the expected nil return before the subsequent require.NoError call; keep the early return nil for matches and otherwise let require.NoError(t, err) run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/devsandbox-dashboard/usersignup.go`:
- Around line 70-78: DeleteUserSignupThroughKsctl should first check whether the
UserSignup exists and return early if it does not to avoid failing cleanup; call
the helper GetUserSignupThroughKsctl(t, username) (or run `ksctl get`
equivalent) and if it indicates the resource is absent, log and return instead
of executing `ksctl gdpr-delete`; only run the delete command and assert on its
success when the pre-check shows the UserSignup exists so that cleanup does not
mask earlier test failures.
---
Outside diff comments:
In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go`:
- Around line 43-63: The switch in ensureNoUserSignup does not handle
sandboxui.DevEnv and silently skips cleanup; update ensureNoUserSignup to
explicitly handle sandboxui.DevEnv (either route it to the same cleanup flow as
sandboxui.TestEnv by using testsupport.WaitForDeployments/hostAwait and
sandboxui.WaitForUserSignup/DeleteUserSignup, or return an explicit failure for
unsupported envs), and apply the same explicit handling to the other similar
switch later in the file (the second occurrence around the teardown logic) so
DevEnv does not fall through silently.
---
Duplicate comments:
In `@testsupport/devsandbox-dashboard/usersignup.go`:
- Around line 54-60: The current check only matches the exact substring "Error
from server (NotFound)" and should be made more resilient: in the block inside
DeleteUserSignupThroughKsctl()'s helper (the if err != nil { ... } section
referencing output), replace the strict match with a case-insensitive/looser
check (e.g., lowercase output and check for "notfound" or "not found", or a
case-insensitive regex like `(?i)not\s*found`) so variants of ksctl/ kubectl
missing-resource messages are treated as the expected nil return before the
subsequent require.NoError call; keep the early return nil for matches and
otherwise let require.NoError(t, err) run.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 33710e6d-96b0-40cd-b4d7-b1e5c776a241
📒 Files selected for processing (6)
test/e2e/devsandbox-dashboard/README.mdtest/e2e/devsandbox-dashboard/fresh_signup_test.gotestsupport/devsandbox-dashboard/login.gotestsupport/devsandbox-dashboard/setup.gotestsupport/devsandbox-dashboard/trace.gotestsupport/devsandbox-dashboard/usersignup.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/devsandbox-dashboard/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- testsupport/devsandbox-dashboard/setup.go
- testsupport/devsandbox-dashboard/login.go
- testsupport/devsandbox-dashboard/trace.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/devsandbox-dashboard/fresh_signup_test.go (1)
44-67: Consider adding a default case for unexpected environments.If
ENVIRONMENTis misconfigured to something other thantestorprod, this switch silently does nothing, potentially leaving stale UserSignup resources and causing non-deterministic test behavior.Suggested improvement
case sandboxui.ProdEnv: userSignup := sandboxui.GetUserSignupThroughKsctl(t, username) if userSignup != nil { // delete user signup sandboxui.DeleteUserSignupThroughKsctl(t, username) _, err := page.Reload() require.NoError(t, err) } + default: + t.Logf("Warning: unknown environment %q, skipping UserSignup cleanup", env) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go` around lines 44 - 67, The switch on env (switch env { case sandboxui.TestEnv: ... case sandboxui.ProdEnv: ... }) lacks a default branch so an unexpected ENVIRONMENT value will skip cleanup; add a default case that fails the test or at minimum logs and calls t.Fatalf/require.Failf with the unexpected env value to stop the test and surface the misconfiguration; reference the existing functions (sandboxui.WaitForUserSignup / sandboxui.GetUserSignupThroughKsctl and sandboxui.DeleteUserSignup / sandboxui.DeleteUserSignupThroughKsctl) so the default case occurs alongside those branches and prevents silent no-ops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go`:
- Around line 44-67: The switch on env (switch env { case sandboxui.TestEnv: ...
case sandboxui.ProdEnv: ... }) lacks a default branch so an unexpected
ENVIRONMENT value will skip cleanup; add a default case that fails the test or
at minimum logs and calls t.Fatalf/require.Failf with the unexpected env value
to stop the test and surface the misconfiguration; reference the existing
functions (sandboxui.WaitForUserSignup / sandboxui.GetUserSignupThroughKsctl and
sandboxui.DeleteUserSignup / sandboxui.DeleteUserSignupThroughKsctl) so the
default case occurs alongside those branches and prevents silent no-ops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c08ffe2a-40cd-4888-b9a8-b4834e245f3f
📒 Files selected for processing (1)
test/e2e/devsandbox-dashboard/fresh_signup_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/devsandbox-dashboard/fresh_signup_test.go (1)
44-67: Fail fast on unsupportedENVIRONMENTvalues.These branches now silently do nothing for unknown envs, which can skip cleanup/setup and later fail in the wrong assertion path. A
defaultcase withrequire.FailNowwould make misconfiguration obvious.Proposed fix
switch env { case sandboxui.TestEnv: ... case sandboxui.ProdEnv: ... + default: + require.FailNow(t, "unsupported ENVIRONMENT", "got %q", env) }Also applies to: 141-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go` around lines 44 - 67, The switch on env (comparing against sandboxui.TestEnv and sandboxui.ProdEnv) currently ignores unknown values; update both switch statements that branch on env to include a default case that calls require.FailNow(t, "unsupported ENVIRONMENT: %s", env) (or equivalent require.FailNow usage) so misconfiguration fails immediately; locate the switches that reference env, sandboxui.TestEnv, and sandboxui.ProdEnv (including the similar switch further down) and add the default require.FailNow handling to each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go`:
- Around line 214-217: The prod redirect branch currently calls
devSandboxPage.WaitForURL with a 30s timeout which can hard-fail; update the
call to use the same extended timeout as the subsequent assertion (e.g., 180000
ms / 3 minutes) by changing the playwright.PageWaitForURLOptions Timeout passed
to devSandboxPage.WaitForURL (the call that follows the
strings.Contains(devSandboxPage.URL(), "/oauth/authorize") check) so the
OAuth/console startup path waits long enough before failing.
- Around line 59-65: The cleanup path in ProdEnv currently calls
sandboxui.GetUserSignupThroughKsctl and sandboxui.DeleteUserSignupThroughKsctl
unconditionally; add a guard that inspects the returned UserSignup object's
labels/annotations (e.g., a test-only label like "test-suite=true" or an
annotation "tests.openshift.io/test-account=true") and only call
sandboxui.DeleteUserSignupThroughKsctl if that label/annotation is present,
otherwise log/return an error and skip deletion; apply the same validation
before the second delete call at the other occurrence so both code paths verify
the resource is a test account before deleting.
---
Nitpick comments:
In `@test/e2e/devsandbox-dashboard/fresh_signup_test.go`:
- Around line 44-67: The switch on env (comparing against sandboxui.TestEnv and
sandboxui.ProdEnv) currently ignores unknown values; update both switch
statements that branch on env to include a default case that calls
require.FailNow(t, "unsupported ENVIRONMENT: %s", env) (or equivalent
require.FailNow usage) so misconfiguration fails immediately; locate the
switches that reference env, sandboxui.TestEnv, and sandboxui.ProdEnv (including
the similar switch further down) and add the default require.FailNow handling to
each.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 708969ca-e8f9-4bed-8420-90acf4951622
📒 Files selected for processing (1)
test/e2e/devsandbox-dashboard/fresh_signup_test.go
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Nice Job 👏
Sorry for the slow response!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, rsoaresd 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 |
|
4 participants
Description
Adapt UI E2E tests to run properly against production environment
Issue ticket number and link
SANDBOX-1630
Summary by CodeRabbit
New Features
Documentation
Tests