-
Notifications
You must be signed in to change notification settings - Fork 94
fix(#2490): add retry logic for flaky e2e TestAdminInstallUninstall #2492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,7 +164,24 @@ func TestAdminInstallUninstall(t *testing.T) { | |
| require.NoError(t, err, "vendored marker .defaults/action.yml should exist") | ||
| _, err = env.client.GetFileContent(ctx, env.org, forge.ConfigRepoName, layers.VendoredBinaryPath) | ||
| require.NoError(t, err, "vendored binary should exist at %s", layers.VendoredBinaryPath) | ||
| analyzeOutput := runCLI(t, env.binary, env.token, "admin", "analyze", env.org) | ||
| // Retry analyze with backoff to handle transient 401s from GitHub | ||
| // propagation delays after repo creation (see #2490). | ||
| var analyzeOutput string | ||
| for attempt := range 3 { | ||
| if attempt > 0 { | ||
| delay := time.Duration(attempt*10) * time.Second | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] retry-logging The analyze retry log message uses 'Analyze attempt %d failed, retrying in %s...' which differs from the 'Attempt %d: ' format used by other retry loops in the same file (including the shim verification retry added in this same PR). Suggested fix: Align with the established 'Attempt %d: ' log format for consistency within the file. |
||
| t.Logf("Analyze attempt %d failed, retrying in %s...", attempt, delay) | ||
| time.Sleep(delay) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case If tryRunCLI succeeds but returns an empty string for analyzeOutput, the test proceeds silently. This is pre-existing behavior (the original runCLI call had the same property), not a regression. |
||
| out, analyzeErr := tryRunCLI(t, env.binary, env.token, "admin", "analyze", env.org) | ||
| if analyzeErr == nil { | ||
| analyzeOutput = out | ||
| break | ||
| } | ||
| if attempt == 2 { | ||
| t.Fatalf("admin analyze failed after %d attempts: %v", attempt+1, analyzeErr) | ||
| } | ||
| } | ||
| t.Logf("Analyze output:\n%s", analyzeOutput) | ||
|
|
||
| // Standalone install vendors reusable workflows, actions, and agent content | ||
|
|
@@ -205,7 +222,24 @@ func TestAdminInstallUninstall(t *testing.T) { | |
| mergeEnrollmentPR(t, env) | ||
|
|
||
| // Phase 3: Triage dispatch smoke test. | ||
| // Verify the shim workflow is present on the default branch before | ||
| // creating the test issue. GitHub may take a few seconds after the | ||
| // merge to make the file available via the contents API (#2490). | ||
| t.Log("=== Phase 3: Triage Dispatch Smoke Test ===") | ||
| t.Log("Verifying shim workflow is on default branch...") | ||
| shimVerified := false | ||
| for attempt := range 5 { | ||
| if attempt > 0 { | ||
| time.Sleep(3 * time.Second) | ||
| } | ||
| _, shimErr := env.client.GetFileContent(ctx, env.org, testRepo, ".github/workflows/fullsend.yaml") | ||
| if shimErr == nil { | ||
| shimVerified = true | ||
| break | ||
| } | ||
| t.Logf("Attempt %d: shim workflow not yet visible on default branch: %v", attempt+1, shimErr) | ||
| } | ||
| require.True(t, shimVerified, "shim workflow should be on default branch before triage test") | ||
| runTriageDispatchSmokeTest(t, env) | ||
|
|
||
| // Phase 4: Unenrollment reconciliation. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] retry-pattern-consistency
The analyze retry uses increasing backoff (10s, 20s) while other retries in the file use fixed 3-5s delays. The variation is justified by the different failure mode (401 credential propagation vs. resource availability).