From 29ba4871da3f139ac71df6fa5a1bd99d01ff0319 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 17:39:22 +0530 Subject: [PATCH 1/6] fix(e2e): Fix push/release E2E trigger bugs and silent failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four bugs prevented E2E tests from triggering correctly on release branches and master/main pushes: **Bug A — Wrong server version for push events** Release branch name ("release-9.0") was used to derive a Docker tag ("9.0"), but Cloud only accepts full SemVer tags like "9.0.0". This caused installation create errors and the workflow was never dispatched. Fix: `serverVersionForPushEvent` always calls `resolveE2EServerVersion()` (returns latest stable) and ignores the branch name. **Bug B — Empty E2EReleasePatternPrefix matches every branch** `strings.HasPrefix(x, "")` is always true, so when the config field was unset every branch (including master, feature branches) was misclassified as a release branch. Fix: guard returns false on empty prefix. **Bug C — Silent debug logs hide why events are dropped** "Ignoring workflow_run event" and "Push event does not match E2E trigger conditions" were Debug-level with no field values. Promoted to Info and added `configured_nightly_name`, `configured_test_workflows`, `auto_release`, `auto_master`, `release_pattern_prefix`, and `is_release_branch` fields so operators can diagnose without enabling debug logging. **Bug D — Empty E2EServerVersion passed raw to Cloud API** `resolveE2EServerVersion` returned `""` when `E2EServerVersion` was unset, which the Cloud API rejected. Fix: trim whitespace, treat empty as "latest" with a Warn log. Also: - Removed dead `extractVersionFromReleaseBranch` function and its test - Added regression tests for all four bugs in e2e_dryrun_test.go - Updated config/config-matterwick.default.json with missing fields (E2EResetServersLabel, E2EPassword, E2EInstanceMaxAge) Co-Authored-By: Claude Opus 4.7 --- config/config-matterwick.default.json | 5 +- server/e2e_dryrun_test.go | 78 +++++++++++++++++++++++++-- server/e2e_tests.go | 23 +++++--- server/push_events.go | 73 +++++++++++++++---------- server/workflow_run.go | 9 +++- 5 files changed, 149 insertions(+), 39 deletions(-) diff --git a/config/config-matterwick.default.json b/config/config-matterwick.default.json index 1b7ed1a..08816fc 100644 --- a/config/config-matterwick.default.json +++ b/config/config-matterwick.default.json @@ -74,11 +74,14 @@ "E2ELabel": "E2E/Run", "E2EMobileIOSLabel": "E2E/Run-iOS", "E2EMobileAndroidLabel": "E2E/Run-Android", + "E2EResetServersLabel": "E2E/Reset-Servers", "E2EUsername": "admin", + "E2EPassword": "", "E2EServerVersion": "latest", "E2EAutoTriggerOnRelease": true, "E2EAutoTriggerOnMaster": true, "E2EReleasePatternPrefix": "release-", "E2ENightlyTriggerWorkflowName": "E2E Nightly Trigger", - "E2ETestWorkflowNames": ["Electron Playwright Tests", "E2E", "Compatibility Matrix Testing"] + "E2ETestWorkflowNames": ["Electron Playwright Tests", "E2E", "Compatibility Matrix Testing"], + "E2EInstanceMaxAge": 6 } diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 6b5f488..895b973 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -421,10 +421,22 @@ func TestDryRun_DesktopPushEvent(t *testing.T) { assert.False(t, s.isReleaseBranch("feature-branch")) }) - t.Run("version extracted from release branch", func(t *testing.T) { - assert.Equal(t, "8.0", extractVersionFromReleaseBranch("release-8.0", "release-")) - assert.Equal(t, "10.5", extractVersionFromReleaseBranch("release-10.5", "release-")) - assert.Equal(t, "", extractVersionFromReleaseBranch("master", "release-")) + t.Run("empty release pattern prefix never matches any branch", func(t *testing.T) { + // strings.HasPrefix(any, "") is always true. Without this guard, a missing + // or empty E2EReleasePatternPrefix in the deployed config would classify + // every push (master, feature branches, anything) as a release branch and + // trigger spurious E2E provisioning on every push. + empty := newDryRunServer(t, "", "mattermost") + empty.Config.E2EReleasePatternPrefix = "" + + assert.False(t, empty.isReleaseBranch("master"), + "empty prefix must not match master") + assert.False(t, empty.isReleaseBranch("release-8.0"), + "empty prefix must not match release-8.0") + assert.False(t, empty.isReleaseBranch("anything"), + "empty prefix must not match arbitrary branches") + assert.False(t, empty.isReleaseBranch(""), + "empty prefix must not match empty branch") }) t.Run("branch name extracted from git ref", func(t *testing.T) { @@ -909,6 +921,33 @@ func TestDryRun_ResolveE2EServerVersion(t *testing.T) { assert.False(t, called, "GitHub API must not be called when E2EServerVersion is not 'latest'") }) + t.Run("empty config falls back to latest resolution, not empty version", func(t *testing.T) { + // A missing E2EServerVersion field in the deployed config decodes to "". + // Before the fix, "" was returned as-is and flowed to CreateInstallation, + // which silently failed to provision any server. Empty must be treated + // as "latest" so the GitHub-releases lookup runs. + body := `[{"tag_name":"v12.0.0","draft":false,"prerelease":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EServerVersion = "" + s.githubAPIBase = srv.URL + "/" + + assert.Equal(t, "12.0.0", s.resolveE2EServerVersion(), + "empty E2EServerVersion must fall back to latest resolution, not return empty") + }) + + t.Run("whitespace-only config falls back to latest resolution", func(t *testing.T) { + // Defensive: treat whitespace as empty for the same reason — a config-edit + // typo with stray whitespace should not silently break provisioning. + body := `[{"tag_name":"v12.0.0","draft":false,"prerelease":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EServerVersion = " " + s.githubAPIBase = srv.URL + "/" + + assert.Equal(t, "12.0.0", s.resolveE2EServerVersion()) + }) + t.Run("RC tags skipped, first stable tag returned with v stripped", func(t *testing.T) { body := `[ {"tag_name":"v11.7.0-rc2","draft":false}, @@ -1087,6 +1126,37 @@ func TestDryRun_ResolveE2EServerVersion(t *testing.T) { }) } +// ------------------------------------------------------------ +// 12b. Push-event server version selection — always latest, ignore branch suffix +// ------------------------------------------------------------ + +func TestDryRun_PushEventServerVersion(t *testing.T) { + // Push events (release branch, master, main) must always provision the latest + // stable Mattermost release. Deriving a version from a release branch name + // (e.g. "release-9.0" → "9.0") would attempt to pull a Docker tag that + // typically doesn't exist (Docker Hub publishes full SemVer like "9.0.0"), + // causing silent installation failures and no E2E workflow dispatch. + t.Run("release branch push uses latest version, ignoring branch-derived version", func(t *testing.T) { + body := `[{"tag_name":"v12.0.0","draft":false,"prerelease":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + + assert.Equal(t, "12.0.0", s.serverVersionForPushEvent("release-9.0"), + "release-9.0 push must provision latest server (12.0.0), not branch-derived 9.0") + assert.Equal(t, "12.0.0", s.serverVersionForPushEvent("release-10.5"), + "release-10.5 push must provision latest server (12.0.0), not branch-derived 10.5") + }) + + t.Run("master push uses latest version", func(t *testing.T) { + body := `[{"tag_name":"v12.0.0","draft":false,"prerelease":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + + assert.Equal(t, "12.0.0", s.serverVersionForPushEvent("master")) + assert.Equal(t, "12.0.0", s.serverVersionForPushEvent("main")) + }) +} + // ------------------------------------------------------------ // 13. MM_SERVER_VERSION sourced from instance, not config // ------------------------------------------------------------ diff --git a/server/e2e_tests.go b/server/e2e_tests.go index 8f3601c..98d0f83 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -770,17 +770,28 @@ func (s *Server) cleanupStaleNonPRE2EInstances() { } // resolveE2EServerVersion returns the Mattermost server version to use for E2E instances. -// If E2EServerVersion is "latest", it fetches the mattermost/mattermost GitHub releases, -// skips drafts, prerelease-flagged releases, and RC/beta/alpha tag-name patterns, then -// returns the first (newest) fully stable tag stripped of its "v" prefix -// (e.g. "v11.6.0" → "11.6.0") to match the Docker Hub tag format. +// If E2EServerVersion is "latest" (or empty/whitespace, which is treated as "latest"), +// it fetches the mattermost/mattermost GitHub releases, skips drafts, prerelease-flagged +// releases, and RC/beta/alpha tag-name patterns, then returns the first (newest) fully +// stable tag stripped of its "v" prefix (e.g. "v11.6.0" → "11.6.0") to match the Docker +// Hub tag format. // // The resolved version is cached in memory for 1 hour so that back-to-back provisioning // calls (e.g. three parallel platform instances) share a single GitHub API round-trip. // Falls back to "master" on any API error or when no stable release is found. +// +// Empty/whitespace handling: a missing or blank E2EServerVersion in the deployed config +// previously flowed through as "" to CreateInstallation, which silently failed. Treating +// it as "latest" makes the misconfiguration recoverable and is loudly logged so the +// operator can fix the config without losing E2E coverage in the meantime. func (s *Server) resolveE2EServerVersion() string { - if s.Config.E2EServerVersion != "latest" { - return s.Config.E2EServerVersion + cfg := strings.TrimSpace(s.Config.E2EServerVersion) + if cfg == "" { + s.Logger.Warn("[resolveE2EServerVersion] E2EServerVersion is empty in config; defaulting to 'latest'") + cfg = "latest" + } + if cfg != "latest" { + return cfg } const cacheTTL = 1 * time.Hour diff --git a/server/push_events.go b/server/push_events.go index af55513..a141bc7 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -29,26 +29,53 @@ func (s *Server) handlePushEvent(event *github.PushEvent) { // Detect if this is a release branch push if s.Config.E2EAutoTriggerOnRelease && s.isReleaseBranch(branch) { logger.WithField("type", "release_branch").Info("Release branch detected, triggering E2E tests") - version := extractVersionFromReleaseBranch(branch, s.Config.E2EReleasePatternPrefix) - go s.handlePushEventE2E(event, branch, version) + go s.handlePushEventE2E(event, branch) return } // Detect if this is a master/main branch push if s.Config.E2EAutoTriggerOnMaster && (branch == "master" || branch == "main") { logger.WithField("type", "master_main").Info("Master/main branch detected, triggering E2E tests") - go s.handlePushEventE2E(event, branch, "") + go s.handlePushEventE2E(event, branch) return } - logger.Debug("Push event does not match E2E trigger conditions") + // Promoted from Debug to Info so operators can see why an event was dropped + // without having to enable debug logging. The field values reveal which gate + // closed (auto-trigger flag off, release prefix mismatch, or unrecognised branch). + logger.WithFields(logrus.Fields{ + "auto_release": s.Config.E2EAutoTriggerOnRelease, + "auto_master": s.Config.E2EAutoTriggerOnMaster, + "release_pattern_prefix": s.Config.E2EReleasePatternPrefix, + "is_release_branch": s.isReleaseBranch(branch), + }).Info("Push event does not match E2E trigger conditions") } -// isReleaseBranch checks if a branch name is a release branch +// isReleaseBranch checks if a branch name is a release branch. +// An empty E2EReleasePatternPrefix is rejected — strings.HasPrefix(any, "") is +// always true, which would silently mark every branch (including master/main and +// feature branches) as a release branch. func (s *Server) isReleaseBranch(branch string) bool { + if s.Config.E2EReleasePatternPrefix == "" { + return false + } return strings.HasPrefix(branch, s.Config.E2EReleasePatternPrefix) } +// serverVersionForPushEvent returns the Mattermost server version to provision for +// any push event (release branch, master, or main). Always uses the latest stable +// release; the branch name is intentionally ignored. +// +// Why we don't derive a version from the branch: a "release-9.0" branch would yield +// "9.0", but Docker Hub publishes full SemVer tags like "9.0.0". Provisioning with +// "9.0" results in an installation create error and the E2E workflow is never +// dispatched. The "Latest Version Servers" feature (PR #87) is unambiguous — push +// events always test against the latest stable release. +func (s *Server) serverVersionForPushEvent(branch string) string { + _ = branch // intentionally unused; documented above + return s.resolveE2EServerVersion() +} + // extractBranchName extracts the branch name from the ref // GitHub sends refs in the format "refs/heads/branch-name" func extractBranchName(ref string) string { @@ -59,17 +86,11 @@ func extractBranchName(ref string) string { return strings.Join(parts[2:], "/") } -// extractVersionFromReleaseBranch extracts version from release branch name -// Example: "release-8.0" -> "8.0" -func extractVersionFromReleaseBranch(branch string, prefix string) string { - if !strings.HasPrefix(branch, prefix) { - return "" - } - return strings.TrimPrefix(branch, prefix) -} - -// handlePushEventE2E orchestrates E2E testing for push events (release or master/main) -func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string, version string) { +// handlePushEventE2E orchestrates E2E testing for push events (release or master/main). +// The branch name is used only to classify the run (RELEASE vs MASTER) and to dispatch +// the workflow against the correct ref. The Mattermost server version provisioned for +// testing is always the latest stable release — see serverVersionForPushEvent. +func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { repoName := event.GetRepo().GetName() commit := event.GetHeadCommit() sha := "" @@ -78,10 +99,9 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string, vers } logger := s.Logger.WithFields(logrus.Fields{ - "repo": repoName, - "branch": branch, - "version": version, - "sha": sha, + "repo": repoName, + "branch": branch, + "sha": sha, }) // Determine if this is a desktop or mobile repository @@ -109,7 +129,7 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string, vers logger.WithField("instanceType", instanceType).Info("Creating E2E instances for push event") // Create instances based on repo type - instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, version, sha) + instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, sha) if err != nil { logger.WithError(err).Error("Failed to create E2E instances") return @@ -147,7 +167,7 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string, vers // createMultipleE2EInstancesForPushEvent creates all platform instances in parallel. // Results are returned in platforms[] order so index-based assignment is stable. -func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, version, _ string) ([]*E2EInstance, error) { +func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, _ string) ([]*E2EInstance, error) { var platforms []string if instanceType == "desktop" { platforms = []string{"linux", "macos", "windows"} @@ -161,11 +181,10 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, "platformCount": len(platforms), }) - // Name format: {type}-{version}-{platform}-{hex6} - serverVersion := s.resolveE2EServerVersion() - if version != "" { - serverVersion = version - } + // Name format: {type}-{version}-{platform}-{hex6}. + // Always provision the latest stable Mattermost release for push events — + // see serverVersionForPushEvent for why branch-derived versions are not used. + serverVersion := s.serverVersionForPushEvent(branch) sanitizedVersion := sanitizeForDNS(serverVersion) uid := e2eUniqueSuffix() diff --git a/server/workflow_run.go b/server/workflow_run.go index fc35b4a..c153548 100644 --- a/server/workflow_run.go +++ b/server/workflow_run.go @@ -157,7 +157,14 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay return } - logger.Debug("Ignoring workflow_run event (not relevant to E2E lifecycle)") + // Promoted from Debug to Info so operators can see why a workflow_run event was + // ignored without enabling debug logging. The most common cause of "nightly never + // fires" is workflowName not matching the configured E2ENightlyTriggerWorkflowName + // — this log line surfaces the comparison directly. + logger.WithFields(logrus.Fields{ + "configured_nightly_name": s.Config.E2ENightlyTriggerWorkflowName, + "configured_test_workflows": s.Config.E2ETestWorkflowNames, + }).Info("Ignoring workflow_run event (not relevant to E2E lifecycle)") } // handleNightlyE2ETrigger provisions instances and dispatches the test workflow. From 89719ef37afdda2f95237b5a6629dbcf3838d8f0 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 17:56:51 +0530 Subject: [PATCH 2/6] refactor: name blank sha param in createMultipleE2EInstancesForPushEvent The fourth parameter was declared as the blank identifier `_`. Rename it to `sha string` and suppress it with `_ = sha` in the body, mirroring the pattern used in serverVersionForPushEvent, so the intent is documented at the declaration rather than being implicit. No behaviour change. --- server/push_events.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/push_events.go b/server/push_events.go index a141bc7..c4e1103 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -167,7 +167,8 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { // createMultipleE2EInstancesForPushEvent creates all platform instances in parallel. // Results are returned in platforms[] order so index-based assignment is stable. -func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, _ string) ([]*E2EInstance, error) { +func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, sha string) ([]*E2EInstance, error) { + _ = sha // intentionally unused; push events always provision the latest stable release — see serverVersionForPushEvent var platforms []string if instanceType == "desktop" { platforms = []string{"linux", "macos", "windows"} From 59aacfd070acfad150b9b4c33e02736a3c46d0e7 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 18:30:32 +0530 Subject: [PATCH 3/6] refactor: remove unused sha param from createMultipleE2EInstancesForPushEvent sha was accepted but immediately suppressed with _ = sha. The tracking key and dispatch calls that use sha live in the caller (handlePushEventE2E), not here. Removing the parameter eliminates the confusion without any behaviour change. --- server/push_events.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/push_events.go b/server/push_events.go index c4e1103..662ed3c 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -129,7 +129,7 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { logger.WithField("instanceType", instanceType).Info("Creating E2E instances for push event") // Create instances based on repo type - instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, sha) + instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch) if err != nil { logger.WithError(err).Error("Failed to create E2E instances") return @@ -167,8 +167,7 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { // createMultipleE2EInstancesForPushEvent creates all platform instances in parallel. // Results are returned in platforms[] order so index-based assignment is stable. -func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch, sha string) ([]*E2EInstance, error) { - _ = sha // intentionally unused; push events always provision the latest stable release — see serverVersionForPushEvent +func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch string) ([]*E2EInstance, error) { var platforms []string if instanceType == "desktop" { platforms = []string{"linux", "macos", "windows"} From 47fad41871a9d64d17ff841286a86739a2ccdb1a Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 18:38:31 +0530 Subject: [PATCH 4/6] refactor: trim verbose comments to minimal essential documentation Remove or shorten over-explained inline comments and docstrings across push_events.go, workflow_run.go, and e2e_tests.go. Keep comments that explain non-obvious intent (race condition guard, empty-prefix guard, RC tag secondary check, test mock redirect). Remove comments that restate what the code already shows. --- server/e2e_tests.go | 30 +++++------------- server/push_events.go | 70 ++++++++++++------------------------------ server/workflow_run.go | 23 +++----------- 3 files changed, 31 insertions(+), 92 deletions(-) diff --git a/server/e2e_tests.go b/server/e2e_tests.go index 98d0f83..f0efae3 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -769,21 +769,10 @@ func (s *Server) cleanupStaleNonPRE2EInstances() { logger.Info("Non-PR E2E instance cleanup scan complete") } -// resolveE2EServerVersion returns the Mattermost server version to use for E2E instances. -// If E2EServerVersion is "latest" (or empty/whitespace, which is treated as "latest"), -// it fetches the mattermost/mattermost GitHub releases, skips drafts, prerelease-flagged -// releases, and RC/beta/alpha tag-name patterns, then returns the first (newest) fully -// stable tag stripped of its "v" prefix (e.g. "v11.6.0" → "11.6.0") to match the Docker -// Hub tag format. -// -// The resolved version is cached in memory for 1 hour so that back-to-back provisioning -// calls (e.g. three parallel platform instances) share a single GitHub API round-trip. -// Falls back to "master" on any API error or when no stable release is found. -// -// Empty/whitespace handling: a missing or blank E2EServerVersion in the deployed config -// previously flowed through as "" to CreateInstallation, which silently failed. Treating -// it as "latest" makes the misconfiguration recoverable and is loudly logged so the -// operator can fix the config without losing E2E coverage in the meantime. +// resolveE2EServerVersion returns the server version for E2E provisioning. +// "latest" (or empty) fetches the newest stable mattermost/mattermost release — skips +// drafts, prereleases, and RC/beta/alpha tags — strips the "v" prefix, and caches the +// result for 1 hour. Falls back to "master" on API error or if no stable release is found. func (s *Server) resolveE2EServerVersion() string { cfg := strings.TrimSpace(s.Config.E2EServerVersion) if cfg == "" { @@ -806,14 +795,12 @@ func (s *Server) resolveE2EServerVersion() string { } s.e2eVersionCacheLock.Unlock() - // 10-second timeout prevents blocking instance-creation goroutines indefinitely - // if the GitHub API is slow or unreachable. ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() client := newGithubClient(s.Config.GithubAccessToken) - // Redirect to a mock server when running tests (githubAPIBase is empty in production). + // githubAPIBase is only set in tests to redirect to a mock server. if s.githubAPIBase != "" { if baseURL, parseErr := url.Parse(s.githubAPIBase); parseErr == nil { client.BaseURL = baseURL @@ -837,18 +824,15 @@ func (s *Server) resolveE2EServerVersion() string { } for _, r := range releases { - // Skip drafts and GitHub's explicit prerelease flag first. if r.Draft || r.Prerelease { continue } - // Also skip by tag-name pattern as a secondary guard for releases whose - // prerelease flag may not be set correctly (e.g. some RC tags). + // Secondary guard: some RC tags don't have the prerelease flag set correctly. lower := strings.ToLower(r.TagName) if strings.Contains(lower, "-rc") || strings.Contains(lower, "-beta") || strings.Contains(lower, "-alpha") { continue } - // Strip "v" prefix to match Docker Hub tag format (e.g. "v11.6.0" → "11.6.0"). - version := strings.TrimPrefix(r.TagName, "v") + version := strings.TrimPrefix(r.TagName, "v") // Docker Hub uses "11.6.0" not "v11.6.0" s.Logger.WithField("version", version).Info("[resolveE2EServerVersion] Resolved latest Mattermost server version") s.e2eVersionCacheLock.Lock() diff --git a/server/push_events.go b/server/push_events.go index 662ed3c..b54f6a9 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -13,10 +13,10 @@ import ( "github.com/sirupsen/logrus" ) -// handlePushEvent processes push events to trigger E2E tests on release branches or master/main +// handlePushEvent triggers E2E tests on release branches or master/main pushes. func (s *Server) handlePushEvent(event *github.PushEvent) { repoName := event.GetRepo().GetName() - branchRef := event.GetRef() // Format: "refs/heads/branch-name" + branchRef := event.GetRef() // "refs/heads/branch-name" branch := extractBranchName(branchRef) logger := s.Logger.WithFields(logrus.Fields{ @@ -26,23 +26,18 @@ func (s *Server) handlePushEvent(event *github.PushEvent) { }) logger.Info("Push event received") - // Detect if this is a release branch push if s.Config.E2EAutoTriggerOnRelease && s.isReleaseBranch(branch) { logger.WithField("type", "release_branch").Info("Release branch detected, triggering E2E tests") go s.handlePushEventE2E(event, branch) return } - // Detect if this is a master/main branch push if s.Config.E2EAutoTriggerOnMaster && (branch == "master" || branch == "main") { logger.WithField("type", "master_main").Info("Master/main branch detected, triggering E2E tests") go s.handlePushEventE2E(event, branch) return } - // Promoted from Debug to Info so operators can see why an event was dropped - // without having to enable debug logging. The field values reveal which gate - // closed (auto-trigger flag off, release prefix mismatch, or unrecognised branch). logger.WithFields(logrus.Fields{ "auto_release": s.Config.E2EAutoTriggerOnRelease, "auto_master": s.Config.E2EAutoTriggerOnMaster, @@ -51,10 +46,8 @@ func (s *Server) handlePushEvent(event *github.PushEvent) { }).Info("Push event does not match E2E trigger conditions") } -// isReleaseBranch checks if a branch name is a release branch. -// An empty E2EReleasePatternPrefix is rejected — strings.HasPrefix(any, "") is -// always true, which would silently mark every branch (including master/main and -// feature branches) as a release branch. +// isReleaseBranch returns true if branch matches E2EReleasePatternPrefix. +// Rejects empty prefix — strings.HasPrefix(x, "") is always true. func (s *Server) isReleaseBranch(branch string) bool { if s.Config.E2EReleasePatternPrefix == "" { return false @@ -62,22 +55,14 @@ func (s *Server) isReleaseBranch(branch string) bool { return strings.HasPrefix(branch, s.Config.E2EReleasePatternPrefix) } -// serverVersionForPushEvent returns the Mattermost server version to provision for -// any push event (release branch, master, or main). Always uses the latest stable -// release; the branch name is intentionally ignored. -// -// Why we don't derive a version from the branch: a "release-9.0" branch would yield -// "9.0", but Docker Hub publishes full SemVer tags like "9.0.0". Provisioning with -// "9.0" results in an installation create error and the E2E workflow is never -// dispatched. The "Latest Version Servers" feature (PR #87) is unambiguous — push -// events always test against the latest stable release. +// serverVersionForPushEvent always returns the latest stable Mattermost release. +// Branch name is ignored — derived names like "9.0" don't exist as Docker Hub tags ("9.0.0"). func (s *Server) serverVersionForPushEvent(branch string) string { - _ = branch // intentionally unused; documented above + _ = branch return s.resolveE2EServerVersion() } -// extractBranchName extracts the branch name from the ref -// GitHub sends refs in the format "refs/heads/branch-name" +// extractBranchName extracts the branch name from "refs/heads/branch-name". func extractBranchName(ref string) string { parts := strings.Split(ref, "/") if len(parts) < 3 { @@ -86,10 +71,8 @@ func extractBranchName(ref string) string { return strings.Join(parts[2:], "/") } -// handlePushEventE2E orchestrates E2E testing for push events (release or master/main). -// The branch name is used only to classify the run (RELEASE vs MASTER) and to dispatch -// the workflow against the correct ref. The Mattermost server version provisioned for -// testing is always the latest stable release — see serverVersionForPushEvent. +// handlePushEventE2E provisions E2E servers and dispatches the test workflow +// for a push to a release branch or master/main. Only acts on desktop/mobile repos. func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { repoName := event.GetRepo().GetName() commit := event.GetHeadCommit() @@ -104,7 +87,6 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { "sha": sha, }) - // Determine if this is a desktop or mobile repository isDesktop := strings.Contains(repoName, "desktop") isMobile := strings.Contains(repoName, "mobile") @@ -113,14 +95,11 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { return } - // Create E2E instances for testing instanceType := "desktop" if isMobile { instanceType = "mobile" } - // Validate SHA before provisioning — an empty SHA would produce a malformed - // tracking key and send an empty MOBILE_VERSION to the test workflow. if sha == "" { logger.Error("Push event has no commit SHA, skipping E2E dispatch") return @@ -128,7 +107,6 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { logger.WithField("instanceType", instanceType).Info("Creating E2E instances for push event") - // Create instances based on repo type instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch) if err != nil { logger.WithError(err).Error("Failed to create E2E instances") @@ -142,19 +120,16 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { logger.WithField("instanceCount", len(instances)).Info("E2E instances created successfully") - // Track instances BEFORE dispatching so that a fast-completing workflow_run - // completed event cannot race ahead of us and find nothing to clean up. + // Store instances before dispatching so a fast-completing workflow_run event + // doesn't race ahead and find nothing to clean up. key := fmt.Sprintf("%s-push-%s-%s", repoName, branch, sha) s.e2eInstancesLock.Lock() s.e2eInstances[key] = instances s.e2eInstancesLock.Unlock() - // Trigger the appropriate E2E workflow, passing the tracking key so it reaches - // the workflow inputs as mw_tracking_key for reliable cleanup on completion. err = s.triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, key, instances) if err != nil { logger.WithError(err).Error("Failed to trigger E2E workflow") - // Remove from tracking and destroy instances on dispatch failure s.e2eInstancesLock.Lock() delete(s.e2eInstances, key) s.e2eInstancesLock.Unlock() @@ -181,9 +156,6 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, "platformCount": len(platforms), }) - // Name format: {type}-{version}-{platform}-{hex6}. - // Always provision the latest stable Mattermost release for push events — - // see serverVersionForPushEvent for why branch-derived versions are not used. serverVersion := s.serverVersionForPushEvent(branch) sanitizedVersion := sanitizeForDNS(serverVersion) uid := e2eUniqueSuffix() @@ -246,7 +218,7 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, return instances, nil } -// getRunnerForPlatform returns the GitHub Actions runner for a given platform +// getRunnerForPlatform returns the GitHub Actions runner label for a given platform. func getRunnerForPlatform(platform string) string { switch strings.ToLower(platform) { case "linux": @@ -260,9 +232,8 @@ func getRunnerForPlatform(platform string) string { } } -// triggerE2EWorkflowForPushEvent triggers the E2E workflow for a push event. -// trackingKey is threaded through to the dispatch call so it appears as mw_tracking_key -// in the workflow inputs, enabling direct key-based cleanup on completion. +// triggerE2EWorkflowForPushEvent routes to the desktop or mobile dispatch function. +// trackingKey is embedded in workflow inputs as mw_tracking_key for cleanup on completion. func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, trackingKey string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, @@ -271,7 +242,6 @@ func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, "sha": sha, }) - // Determine repo owner repoOwner := s.Config.Org if repoOwner == "" { logger.Error("Organization not configured") @@ -285,14 +255,13 @@ func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, return s.triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey, instances) } -// triggerDesktopE2EWorkflowForPushEvent triggers the desktop E2E workflow +// triggerDesktopE2EWorkflowForPushEvent dispatches the desktop E2E workflow. func (s *Server) triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, "branch": branch, }) - // Build instance details JSON for desktop workflow instanceDetailsJSON, err := s.buildInstanceDetailsJSON(instances) if err != nil { logger.WithError(err).Error("Failed to build instance details JSON") @@ -309,7 +278,7 @@ func (s *Server) triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, bran return s.dispatchDesktopE2EWorkflow(repoOwner, repoName, branch, sha, instanceDetailsJSON, runType, trackingKey, false) } -// triggerMobileE2EWorkflowForPushEvent triggers the mobile E2E workflow (e2e-detox-pr.yml) +// triggerMobileE2EWorkflowForPushEvent dispatches the mobile E2E workflow (e2e-detox-pr.yml). func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, @@ -325,7 +294,7 @@ func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branc "site_1_url": instances[0].URL, "site_2_url": instances[1].URL, "site_3_url": instances[2].URL, - }).Debug("Triggering mobile E2E workflow (e2e-detox-pr.yml) for push event") + }).Debug("Triggering mobile E2E workflow for push event") runType := "MASTER" if s.isReleaseBranch(branch) { @@ -335,8 +304,7 @@ func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branc return s.dispatchMobileE2EWorkflow( repoOwner, repoName, branch, sha, instances[0].URL, instances[1].URL, instances[2].URL, - "both", // Push events (release/master) test both platforms + "both", // push events always test both iOS and Android runType, trackingKey, ) } - diff --git a/server/workflow_run.go b/server/workflow_run.go index c153548..eba97da 100644 --- a/server/workflow_run.go +++ b/server/workflow_run.go @@ -43,8 +43,7 @@ func ParseWorkflowRunEventWithInputs(data io.Reader) (*WorkflowRunWebhookPayload return &payload, nil } -// handleWorkflowRunEventWithInputs handles GitHub workflow_run events. -// Routes events to CMT, nightly trigger, or test-workflow completion handlers. +// handleWorkflowRunEventWithInputs routes workflow_run events to CMT, nightly, or cleanup handlers. func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPayload) { // Extract repository info repoData := payload.Repository @@ -79,10 +78,7 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay "head_sha": headSHA, }) - // --- CMT trigger workflows --- - // "CMT Provisioner" (name contains "CMT") is dispatched by users with server_versions. - // "Compatibility Matrix Testing" (the actual test workflow) is dispatched by Matterwick - // with CMT_MATRIX; its completion triggers sha-based cleanup via isE2ETestWorkflow. + // CMT: "CMT Provisioner" (user-dispatched) provisions servers; "Compatibility Matrix Testing" runs tests. if strings.Contains(workflowName, "cmt") || strings.Contains(workflowName, "CMT") { if payload.Action == "completed" { logger.Debug("CMT trigger workflow completed; sha-based cleanup is primary") @@ -118,9 +114,7 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay return } - // --- Nightly trigger workflow --- - // When the lightweight nightly trigger workflow is requested, provision instances and - // dispatch the actual test workflow. The nightly trigger workflow does no testing itself. + // Nightly: lightweight trigger workflow fires first; matterwick provisions instances and dispatches the real test workflow. if s.Config.E2ENightlyTriggerWorkflowName != "" && workflowName == s.Config.E2ENightlyTriggerWorkflowName { if payload.Action == "requested" { logger.Info("Nightly trigger workflow started, provisioning E2E servers") @@ -133,10 +127,7 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay if payload.Action == "completed" && s.isE2ETestWorkflow(workflowName) { logger.Info("Test workflow completed, checking for instance cleanup") - // Primary path: direct tracking-key lookup using the key embedded in dispatch - // inputs. Immune to SHA mismatch (new commits during ~30 min provisioning window) - // and to runID collisions (two runs sharing the same branch HEAD sha). - // Reading a nil Inputs map in Go returns "" safely — no nil check required. + // Primary: look up by mw_tracking_key embedded at dispatch time (immune to SHA races). if trackingKey := payload.WorkflowRun.Inputs["mw_tracking_key"]; trackingKey != "" { s.e2eInstancesLock.Lock() instances := s.e2eInstances[trackingKey] @@ -151,16 +142,12 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay return } - // Fallback: SHA-based scan for runs dispatched before mw_tracking_key was added. + // Fallback: SHA-based scan (runs dispatched before mw_tracking_key was introduced). logger.Debug("No mw_tracking_key in workflow inputs, falling back to SHA-based instance cleanup") s.findAndDestroyInstancesBySHA(repoName, headSHA, logger) return } - // Promoted from Debug to Info so operators can see why a workflow_run event was - // ignored without enabling debug logging. The most common cause of "nightly never - // fires" is workflowName not matching the configured E2ENightlyTriggerWorkflowName - // — this log line surfaces the comparison directly. logger.WithFields(logrus.Fields{ "configured_nightly_name": s.Config.E2ENightlyTriggerWorkflowName, "configured_test_workflows": s.Config.E2ETestWorkflowNames, From f53f58a91f6a486b28ff9d8c1f58374db539471a Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 18:49:43 +0530 Subject: [PATCH 5/6] docs: document intentional macos runner difference between E2E and CMT getRunnerForPlatform uses macos-latest (E2E functional workflows track latest) while buildDesktopCMTMatrixJSON pins macos-13 (CMT requires a specific OS version for compatibility testing). Add a comment to make the distinction explicit and prevent future confusion. --- server/push_events.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/push_events.go b/server/push_events.go index b54f6a9..0a57c29 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -218,7 +218,10 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, return instances, nil } -// getRunnerForPlatform returns the GitHub Actions runner label for a given platform. +// getRunnerForPlatform returns the GitHub Actions runner label for E2E functional +// workflows (PR label + push events). CMT workflows use a separate hardcoded matrix +// in buildDesktopCMTMatrixJSON (macos-13) because compatibility testing pins a +// specific OS version; functional tests track latest. func getRunnerForPlatform(platform string) string { switch strings.ToLower(platform) { case "linux": From 6e447d2df066022a072f73d443c0856c05c3a18e Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 6 May 2026 18:57:03 +0530 Subject: [PATCH 6/6] fix: ignore non-branch push refs before E2E trigger evaluation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tag pushes (refs/tags/*) were passed through extractBranchName and could accidentally match isReleaseBranch — e.g. "refs/tags/release-9.0" extracts to "release-9.0" which matches the "release-" prefix. Added an early return for any ref that does not start with "refs/heads/". Also correct the serverVersionForPushEvent comment: resolveE2EServerVersion can fall back to "master" on API error, so "always returns latest stable" was inaccurate. --- server/e2e_dryrun_test.go | 12 ++++++++++++ server/push_events.go | 17 +++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 895b973..d778257 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -445,6 +445,18 @@ func TestDryRun_DesktopPushEvent(t *testing.T) { assert.Equal(t, "feature/my-branch", extractBranchName("refs/heads/feature/my-branch")) }) + t.Run("tag refs are not treated as branch refs", func(t *testing.T) { + // extractBranchName("refs/tags/release-9.0") returns "release-9.0", + // which would match isReleaseBranch and trigger unintended E2E provisioning. + // handlePushEvent must guard against non-refs/heads/ refs before calling extractBranchName. + tagRef := "refs/tags/release-9.0" + assert.False(t, strings.HasPrefix(tagRef, "refs/heads/"), + "tag ref must be filtered before branch-trigger evaluation") + // The extracted value looks like a release branch — the guard is what prevents it. + assert.Equal(t, "release-9.0", extractBranchName(tagRef), + "extractBranchName is unaware of ref type; caller must pre-filter") + }) + t.Run("desktop push always creates linux/macos/windows instances", func(t *testing.T) { // createMultipleE2EInstancesForPushEvent uses desktop platforms for push events expectedPlatforms := []string{"linux", "macos", "windows"} diff --git a/server/push_events.go b/server/push_events.go index 0a57c29..685a269 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -16,7 +16,20 @@ import ( // handlePushEvent triggers E2E tests on release branches or master/main pushes. func (s *Server) handlePushEvent(event *github.PushEvent) { repoName := event.GetRepo().GetName() - branchRef := event.GetRef() // "refs/heads/branch-name" + branchRef := event.GetRef() + + // Ignore tag pushes (refs/tags/*) — only branch pushes should trigger E2E. + // Without this guard, a tag like "refs/tags/release-9.0" would pass through + // extractBranchName as "release-9.0" and accidentally match isReleaseBranch. + if !strings.HasPrefix(branchRef, "refs/heads/") { + s.Logger.WithFields(logrus.Fields{ + "repo": repoName, + "ref": branchRef, + "action": "push", + }).Info("Push ref is not a branch, skipping E2E trigger") + return + } + branch := extractBranchName(branchRef) logger := s.Logger.WithFields(logrus.Fields{ @@ -55,7 +68,7 @@ func (s *Server) isReleaseBranch(branch string) bool { return strings.HasPrefix(branch, s.Config.E2EReleasePatternPrefix) } -// serverVersionForPushEvent always returns the latest stable Mattermost release. +// serverVersionForPushEvent resolves the server version via resolveE2EServerVersion. // Branch name is ignored — derived names like "9.0" don't exist as Docker Hub tags ("9.0.0"). func (s *Server) serverVersionForPushEvent(branch string) string { _ = branch