test: adapt UI e2e tests to prod environment#1271
Open
rsoaresd wants to merge 5 commits intocodeready-toolchain:masterfrom
Open
test: adapt UI e2e tests to prod environment#1271rsoaresd wants to merge 5 commits intocodeready-toolchain:masterfrom
rsoaresd wants to merge 5 commits intocodeready-toolchain:masterfrom
Conversation
WalkthroughAdds production-capable E2E test support for the Developer Sandbox Dashboard: externalizes environment variables, adds prod Make targets, introduces ksctl-based user-signup management, applies username masking, and adapts test/login/popup flows and verifications for prod OAuth/guided-tour behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant Browser as Browser (Playwright)
participant SSO as SSO/OAuth
participant ksctl as ksctl (CLI)
participant Kube as Kubernetes/API
TestRunner->>Browser: start test, Navigate (maskUsername) -> goto(BASE_URL, timeout=60s)
Browser->>SSO: redirect to /oauth/authorize (popup)
TestRunner->>Browser: ClickAndWaitForPopup (maskUsername), expect popup
Browser->>SSO: perform login (username/password)
SSO-->>Browser: redirect back with project page
Browser->>TestRunner: page shows guided-tour modal
TestRunner->>ksctl: GetUserSignupThroughKsctl(username) (when cleaning)
ksctl->>Kube: query UserSignup resource
Kube-->>ksctl: YAML userSignup or "not found"
TestRunner->>ksctl: DeleteUserSignupThroughKsctl(username) (gdpr-delete, confirm)
ksctl->>Kube: delete UserSignup
Kube-->>ksctl: deletion confirmation (poll until not found)
ksctl-->>TestRunner: command output (deleted)
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
Comment on lines
+67
to
+71
| // 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
Comment on lines
+54
to
+60
| if err != nil { | ||
| // Check if it's a not found error | ||
| if strings.Contains(string(output), "Error from server (NotFound)") { | ||
| return nil | ||
| } | ||
| } | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Make the ksctl NotFound handling consistent and fail fast on other errors.
GetUserSignupThroughKsctl() treats Error from server (NotFound) as missing, but the delete poll only matches lowercase not found and turns every other failure into return false, nil. A successful delete can therefore spin until the two-minute timeout, and real ksctl/KUBECONFIG failures get hidden until the timeout expires.
🛠️ 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)
// parse the output as a UserSignup
userSignup := &toolchainv1alpha1.UserSignup{}
@@
err = k8swait.PollUntilContextTimeout(context.TODO(), time.Second, 2*time.Minute, true,
func(ctx context.Context) (done bool, err error) {
// `#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), "not found") || strings.Contains(err.Error(), "not found") {
+ if ksctlUserSignupNotFound(output, err) {
t.Log("UserSignup successfully deleted")
return true, nil
}
- // Some other error, keep trying
- t.Log("Error checking UserSignup deletion")
- return false, nil
+ return false, err
}
// Still exists
return false, nil
})Also applies to: 87-100
🤖 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,
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.
alexeykazakov
approved these changes
Mar 25, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, 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 |
|
|
@rsoaresd: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adapt UI E2E tests to run properly against production environment
Issue ticket number and link
SANDBOX-1682
Summary by CodeRabbit
New Features
Tests
Documentation
Chores