Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion server/e2e_dryrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,12 @@ func TestDryRun_MobileDispatch(t *testing.T) {
require.Len(t, *captures, 1)

capture := (*captures)[0]
assert.Equal(t, prRef, capture.Ref)
assert.Equal(t, "main", capture.Ref,
"mobile PR E2E must dispatch against default-branch workflow YAML")
assert.Equal(t, prRef, capture.Inputs["version_name"],
"version_name keeps the PR head branch for TSIO reporting")
assert.Equal(t, "42", capture.Inputs["pr_number"])
assert.Equal(t, "PR", capture.Inputs["run_type"])
assert.Equal(t, tt.platform, capture.Inputs["PLATFORM"])
assert.Equal(t, prSha, capture.Inputs["MOBILE_VERSION"])
assert.Equal(t, "https://android-site1.test.example.com", capture.Inputs["ANDROID_SITE_1_URL"])
Expand Down
76 changes: 53 additions & 23 deletions server/e2e_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -701,18 +701,30 @@ func (s *Server) triggerMobileE2EWorkflow(ctx context.Context, client *github.Cl
"MOBILE_VERSION": pr.Sha,
"PLATFORM": testPlatform, // Workflow input: which mobile OS to test (ios/android/both)
"pr_number": fmt.Sprintf("%d", pr.Number),
"version_name": pr.Ref,
"run_type": "PR",
}
for inputKey, url := range mobileInputs {
inputs[inputKey] = url
}

// Use the github REST API to trigger the workflow_dispatch event
// Dispatch against the default branch workflow YAML so release-* / cherry-pick
// PR heads (which may lack five-server inputs) still accept this payload.
// MOBILE_VERSION remains the PR head SHA under test; concurrency + run-name
// on e2e-detox-pr.yml are keyed by pr_number so sibling PRs do not cancel
// each other when they share ref=main.
workflowRef := mobileE2EWorkflowRef()

body := map[string]interface{}{
"ref": pr.Ref,
"ref": workflowRef,
"inputs": inputs,
}

logger.WithField("workflow", "e2e-detox-pr.yml").Debug("Triggering mobile E2E workflow")
logger.WithFields(logrus.Fields{
"workflow": "e2e-detox-pr.yml",
"workflow_ref": workflowRef,
"pr_ref": pr.Ref,
}).Debug("Triggering mobile E2E workflow")

req, err := client.NewRequest("POST", fmt.Sprintf("/repos/%s/%s/actions/workflows/e2e-detox-pr.yml/dispatches", pr.RepoOwner, pr.RepoName), body)
if err != nil {
Expand All @@ -728,6 +740,13 @@ func (s *Server) triggerMobileE2EWorkflow(ctx context.Context, client *github.Cl
return nil
}

// mobileE2EWorkflowRef is the git ref whose e2e-detox-pr.yml is used for PR
// dispatches. Always the mobile default branch so outdated release cherry-pick
// heads still get current workflow inputs (ANDROID_SITE_* / e2e-test/*).
func mobileE2EWorkflowRef() string {
return "main"
}

// handleE2ECleanup destroys tracked E2E instances, then queries the cloud API by DNS pattern to catch orphans.
func (s *Server) handleE2ECleanup(pr *model.PullRequest) {
logger := s.Logger.WithFields(logrus.Fields{
Expand Down Expand Up @@ -1687,9 +1706,10 @@ func (s *Server) cancelPRWorkflowRuns(pr *model.PullRequest, logger logrus.Field

var workflowRuns struct {
WorkflowRuns []struct {
ID int64 `json:"id"`
HeadBranch string `json:"head_branch"`
Status string `json:"status"`
ID int64 `json:"id"`
HeadBranch string `json:"head_branch"`
DisplayTitle string `json:"display_title"`
Status string `json:"status"`
} `json:"workflow_runs"`
}

Expand All @@ -1699,29 +1719,39 @@ func (s *Server) cancelPRWorkflowRuns(pr *model.PullRequest, logger logrus.Field
return
}

// Cancel workflow runs that match this PR's branch
// Mobile PR runs are dispatched with ref=main and run-name "E2E PR <n>", so
// match display_title (not head_branch == pr.Ref). Desktop still matches branch.
prTitlePrefix := fmt.Sprintf("E2E PR %d", pr.Number)
cancelCount := 0
for _, run := range workflowRuns.WorkflowRuns {
// Check if this run is for the PR's branch
if run.HeadBranch == pr.Ref && run.Status == "in_progress" {
cancelURL := fmt.Sprintf("/repos/%s/%s/actions/runs/%d/cancel",
pr.RepoOwner, pr.RepoName, run.ID)
if run.Status != "in_progress" {
continue
}
matches := run.HeadBranch == pr.Ref
if strings.Contains(pr.RepoName, "mobile") {
matches = strings.HasPrefix(run.DisplayTitle, prTitlePrefix)
}
if !matches {
Comment on lines +1722 to +1734

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Prevent cancellation of runs for a different PR.

strings.HasPrefix(run.DisplayTitle, "E2E PR 1") also matches "E2E PR 10". The cancellation can stop an in-progress workflow for another PR.

Match the complete title, or require the title separator that follows the PR number. Add a case for PR 1 and PR 10.

Proposed fix
  prTitlePrefix := fmt.Sprintf("E2E PR %d", pr.Number)
  cancelCount := 0
  for _, run := range workflowRuns.WorkflowRuns {
      if run.Status != "in_progress" {
          continue
      }
      matches := run.HeadBranch == pr.Ref
      if strings.Contains(pr.RepoName, "mobile") {
-         matches = strings.HasPrefix(run.DisplayTitle, prTitlePrefix)
+         matches = run.DisplayTitle == prTitlePrefix ||
+             strings.HasPrefix(run.DisplayTitle, prTitlePrefix+" ")
      }
📝 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.

Suggested change
// Mobile PR runs are dispatched with ref=main and run-name "E2E PR <n>", so
// match display_title (not head_branch == pr.Ref). Desktop still matches branch.
prTitlePrefix := fmt.Sprintf("E2E PR %d", pr.Number)
cancelCount := 0
for _, run := range workflowRuns.WorkflowRuns {
// Check if this run is for the PR's branch
if run.HeadBranch == pr.Ref && run.Status == "in_progress" {
cancelURL := fmt.Sprintf("/repos/%s/%s/actions/runs/%d/cancel",
pr.RepoOwner, pr.RepoName, run.ID)
if run.Status != "in_progress" {
continue
}
matches := run.HeadBranch == pr.Ref
if strings.Contains(pr.RepoName, "mobile") {
matches = strings.HasPrefix(run.DisplayTitle, prTitlePrefix)
}
if !matches {
// Mobile PR runs are dispatched with ref=main and run-name "E2E PR <n>", so
// match display_title (not head_branch == pr.Ref). Desktop still matches branch.
prTitlePrefix := fmt.Sprintf("E2E PR %d", pr.Number)
cancelCount := 0
for _, run := range workflowRuns.WorkflowRuns {
if run.Status != "in_progress" {
continue
}
matches := run.HeadBranch == pr.Ref
if strings.Contains(pr.RepoName, "mobile") {
matches = run.DisplayTitle == prTitlePrefix ||
strings.HasPrefix(run.DisplayTitle, prTitlePrefix+" ")
}
if !matches {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/e2e_tests.go` around lines 1722 - 1734, Update the mobile workflow
matching in the loop using prTitlePrefix so PR numbers match exactly rather than
sharing a prefix; require the complete display title or its valid separator.
Preserve desktop branch matching, and add coverage for PR 1 not matching PR 10.

continue
}

cancelReq, err := client.NewRequest("POST", cancelURL, nil)
if err != nil {
logger.WithError(err).WithField("run_id", run.ID).Error("Failed to create cancel request")
continue
}
cancelURL := fmt.Sprintf("/repos/%s/%s/actions/runs/%d/cancel",
pr.RepoOwner, pr.RepoName, run.ID)

_, err = client.Do(ctx, cancelReq, nil)
if err != nil {
logger.WithError(err).WithField("run_id", run.ID).Error("Failed to cancel workflow run")
continue
}
cancelReq, err := client.NewRequest("POST", cancelURL, nil)
if err != nil {
logger.WithError(err).WithField("run_id", run.ID).Error("Failed to create cancel request")
continue
}

logger.WithField("run_id", run.ID).Info("Cancelled workflow run")
cancelCount++
_, err = client.Do(ctx, cancelReq, nil)
if err != nil {
logger.WithError(err).WithField("run_id", run.ID).Error("Failed to cancel workflow run")
continue
}

logger.WithField("run_id", run.ID).Info("Cancelled workflow run")
cancelCount++
}

if cancelCount > 0 {
Expand Down
Loading