From bcb9df91712cd2b92dedc76cdb00b344790f9335 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 15 Apr 2026 10:48:47 +0530 Subject: [PATCH 1/4] Create Latest Version Servers for Desktop and mobile e2e --- config/config-matterwick.default.json | 2 +- server/e2e_dryrun_test.go | 353 +++++++++++++++++++++++++- server/e2e_tests.go | 59 ++++- server/pull_request.go | 7 +- server/push_events.go | 2 +- server/server.go | 5 + server/workflow_run.go | 4 +- 7 files changed, 422 insertions(+), 10 deletions(-) diff --git a/config/config-matterwick.default.json b/config/config-matterwick.default.json index 3d99cb7..1b7ed1a 100644 --- a/config/config-matterwick.default.json +++ b/config/config-matterwick.default.json @@ -75,7 +75,7 @@ "E2EMobileIOSLabel": "E2E/Run-iOS", "E2EMobileAndroidLabel": "E2E/Run-Android", "E2EUsername": "admin", - "E2EServerVersion": "master", + "E2EServerVersion": "latest", "E2EAutoTriggerOnRelease": true, "E2EAutoTriggerOnMaster": true, "E2EReleasePatternPrefix": "release-", diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 0569bbe..5689684 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -183,14 +183,17 @@ func TestDryRun_DesktopDispatch(t *testing.T) { }) t.Run("workflow inputs built correctly", func(t *testing.T) { + // MM_SERVER_VERSION must come from instances[0].ServerVersion, NOT from + // s.Config.E2EServerVersion. The instance stores the resolved version set at + // provisioning time, which may differ from config (e.g. config="latest"). workflowInputs := map[string]interface{}{ "instance_details": instanceDetailsJSON, "version_name": "feature-branch", "MM_TEST_USER_NAME": s.Config.E2EUsername, - "MM_SERVER_VERSION": s.Config.E2EServerVersion, + "MM_SERVER_VERSION": instances[0].ServerVersion, // NOT s.Config.E2EServerVersion } - assert.Equal(t, "master", workflowInputs["MM_SERVER_VERSION"]) + assert.Equal(t, instances[0].ServerVersion, workflowInputs["MM_SERVER_VERSION"]) assert.Equal(t, "e2eadmin", workflowInputs["MM_TEST_USER_NAME"]) assert.Equal(t, "feature-branch", workflowInputs["version_name"]) assert.NotEmpty(t, workflowInputs["instance_details"]) @@ -829,3 +832,349 @@ func TestDryRun_ConcurrentInstanceAccess(t *testing.T) { wg.Wait() // No race detected → concurrent access is safe } + +// ------------------------------------------------------------ +// 12. resolveE2EServerVersion() — GitHub releases API logic +// ------------------------------------------------------------ + +// mockReleasesServer returns an httptest.Server that serves the given body/status +// for any GET request whose path contains "/releases". +func mockReleasesServer(t *testing.T, body string, status int) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/releases") { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + return srv +} + +// newDryRunServerLatest builds a Server with E2EServerVersion="latest" whose +// GitHub API calls are redirected to mockSrv. +func newDryRunServerLatest(t *testing.T, mockSrv *httptest.Server) *Server { + t.Helper() + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EServerVersion = "latest" + s.githubAPIBase = mockSrv.URL + "/" // must have trailing slash + return s +} + +func TestDryRun_ResolveE2EServerVersion(t *testing.T) { + t.Run("non-latest config returned unchanged, no API call", func(t *testing.T) { + // The mock server should never be hit for non-"latest" configs. + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) + + for _, cfg := range []string{"10.0.0", "master", "9.4.0", "11.6.0"} { + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EServerVersion = cfg + s.githubAPIBase = srv.URL + "/" + assert.Equal(t, cfg, s.resolveE2EServerVersion(), "config=%q should be returned unchanged", cfg) + } + assert.False(t, called, "GitHub API must not be called when E2EServerVersion is not 'latest'") + }) + + 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}, + {"tag_name":"v11.7.0-rc1","draft":false}, + {"tag_name":"v11.6.0","draft":false}, + {"tag_name":"v11.5.0","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("beta tags skipped", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0-beta.1","draft":false}, + {"tag_name":"v11.6.0","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("alpha tags skipped", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0-alpha.1","draft":false}, + {"tag_name":"v11.6.0","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("draft releases skipped", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0","draft":true}, + {"tag_name":"v11.6.0","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("stable release at top of list returned immediately", func(t *testing.T) { + body := `[{"tag_name":"v12.0.0","draft":false},{"tag_name":"v11.6.0","draft":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "12.0.0", s.resolveE2EServerVersion()) + }) + + t.Run("tag without v prefix returned unchanged", func(t *testing.T) { + // TrimPrefix("v", non-v-string) is a no-op — bare semver tags work too. + body := `[{"tag_name":"11.6.0","draft":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("only RCs in list → fallback to master", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0-rc1","draft":false}, + {"tag_name":"v11.6.0-rc2","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "master", s.resolveE2EServerVersion()) + }) + + t.Run("only drafts in list → fallback to master", func(t *testing.T) { + body := `[{"tag_name":"v11.7.0","draft":true}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "master", s.resolveE2EServerVersion()) + }) + + t.Run("empty releases list → fallback to master", func(t *testing.T) { + srv := mockReleasesServer(t, `[]`, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "master", s.resolveE2EServerVersion()) + }) + + t.Run("API returns 500 → fallback to master", func(t *testing.T) { + srv := mockReleasesServer(t, `{"message":"Internal Server Error"}`, http.StatusInternalServerError) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "master", s.resolveE2EServerVersion()) + }) + + t.Run("mixed: draft RC then stable", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0-rc1","draft":true}, + {"tag_name":"v11.7.0-rc1","draft":false}, + {"tag_name":"v11.6.0","draft":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) +} + +// ------------------------------------------------------------ +// 13. MM_SERVER_VERSION sourced from instance, not config +// ------------------------------------------------------------ + +func TestDryRun_MMServerVersionFromInstance(t *testing.T) { + t.Run("desktop dispatch uses instances[0].ServerVersion not config", func(t *testing.T) { + // Simulate the case where config says "latest" but the instance was provisioned + // with the resolved version "11.6.0". The dispatch must use the instance value. + s := newDryRunServer(t, "", "mattermost") + s.Config.E2EServerVersion = "latest" // would be wrong if used in dispatch + + resolvedVersion := "11.6.0" + instances := []*E2EInstance{ + {Name: "inst-linux", Platform: "linux", Runner: "ubuntu-latest", + URL: "https://linux.test.example.com", InstallationID: "id-1", + ServerVersion: resolvedVersion}, + {Name: "inst-macos", Platform: "macos", Runner: "macos-latest", + URL: "https://macos.test.example.com", InstallationID: "id-2", + ServerVersion: resolvedVersion}, + {Name: "inst-windows", Platform: "windows", Runner: "windows-2022", + URL: "https://windows.test.example.com", InstallationID: "id-3", + ServerVersion: resolvedVersion}, + } + + // The desktop workflow dispatch builds inputs using instances[0].ServerVersion. + inputs := map[string]interface{}{ + "MM_SERVER_VERSION": instances[0].ServerVersion, + } + + assert.Equal(t, "11.6.0", inputs["MM_SERVER_VERSION"], + "MM_SERVER_VERSION must be the resolved instance version, not the 'latest' config sentinel") + assert.NotEqual(t, s.Config.E2EServerVersion, inputs["MM_SERVER_VERSION"], + "MM_SERVER_VERSION must NOT be the raw config value when config is 'latest'") + }) + + t.Run("mobile dispatch does not include MM_SERVER_VERSION", func(t *testing.T) { + // Mobile workflows receive SITE_1/2/3_URL and PLATFORM — never MM_SERVER_VERSION. + instances := []*E2EInstance{ + {Name: "inst-site1", Platform: "site-1", + URL: "https://site1.test.example.com", InstallationID: "id-1", + ServerVersion: "11.6.0"}, + {Name: "inst-site2", Platform: "site-2", + URL: "https://site2.test.example.com", InstallationID: "id-2", + ServerVersion: "11.6.0"}, + {Name: "inst-site3", Platform: "site-3", + URL: "https://site3.test.example.com", InstallationID: "id-3", + ServerVersion: "11.6.0"}, + } + + inputs := map[string]interface{}{ + "MOBILE_VERSION": "feature-sha", + "PLATFORM": "both", + "SITE_1_URL": instances[0].URL, + "SITE_2_URL": instances[1].URL, + "SITE_3_URL": instances[2].URL, + } + + assert.NotContains(t, inputs, "MM_SERVER_VERSION", + "mobile dispatch must never include MM_SERVER_VERSION") + assert.NotContains(t, inputs, "instance_details", + "mobile dispatch must never include instance_details") + assert.Equal(t, "https://site1.test.example.com", inputs["SITE_1_URL"]) + assert.Equal(t, "https://site2.test.example.com", inputs["SITE_2_URL"]) + assert.Equal(t, "https://site3.test.example.com", inputs["SITE_3_URL"]) + }) + + t.Run("all instances in a PR run share the same resolved version", func(t *testing.T) { + // createMultipleE2EInstances calls resolveE2EServerVersion() once and passes + // the same version to all createCloudInstallation calls. + resolvedVersion := "11.6.0" + platforms := []string{"linux", "macos", "windows"} + instances := make([]*E2EInstance, len(platforms)) + for i, p := range platforms { + instances[i] = &E2EInstance{ + Platform: p, + ServerVersion: resolvedVersion, // same version for every instance + } + } + for i, inst := range instances { + assert.Equal(t, resolvedVersion, inst.ServerVersion, + "instance[%d] (platform=%s) must have the resolved version", i, inst.Platform) + } + }) + + t.Run("resolveE2EServerVersion with latest returns Docker Hub compatible version", func(t *testing.T) { + // Docker Hub tags are bare semver (e.g. "11.6.0"), NOT "v11.6.0". + // Verify the v-stripping produces a Docker Hub compatible string. + body := `[{"tag_name":"v11.6.0","draft":false}]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + + version := s.resolveE2EServerVersion() + assert.Equal(t, "11.6.0", version) + assert.False(t, strings.HasPrefix(version, "v"), + "resolved version must NOT have 'v' prefix — Docker Hub tags are bare semver") + }) +} + +// ------------------------------------------------------------ +// 14. CMT version normalization (v-prefix stripping) +// ------------------------------------------------------------ + +func TestDryRun_CMTVersionNormalization(t *testing.T) { + t.Run("v-prefix stripped before instance creation", func(t *testing.T) { + // handleCMTWithServerVersions strips "v" from each version before provisioning. + // Verify that strings.TrimPrefix produces Docker Hub compatible versions. + inputs := []struct { + input string + want string + }{ + {"v11.0.1", "11.0.1"}, + {"v11.1.0", "11.1.0"}, + {"v12.0.0", "12.0.0"}, + {"11.0.1", "11.0.1"}, // no v — unchanged + {"11.1.0", "11.1.0"}, // no v — unchanged + {"v11.6.0-rc1", "11.6.0-rc1"}, // RC: v stripped but rest preserved + } + for _, tt := range inputs { + got := strings.TrimPrefix(tt.input, "v") + assert.Equal(t, tt.want, got, "TrimPrefix(%q, 'v')", tt.input) + } + }) + + t.Run("comma-separated input parsed and v-stripped", func(t *testing.T) { + // parseServerVersionsFromString splits; the CMT loop then strips v from each. + raw := "v11.0.1, v11.1.0, 11.2.0" + parsed := parseServerVersionsFromString(raw) + require.Len(t, parsed, 3) + + var stripped []string + for _, v := range parsed { + stripped = append(stripped, strings.TrimPrefix(strings.TrimSpace(v), "v")) + } + assert.Equal(t, []string{"11.0.1", "11.1.0", "11.2.0"}, stripped) + }) + + t.Run("CMT instances carry stripped version in ServerVersion", func(t *testing.T) { + // Instances created by handleCMTWithServerVersions use the stripped version. + // Simulate by constructing instances as the real code would. + rawVersions := []string{"v11.0.1", "v11.1.0"} + var instances []*E2EInstance + for _, v := range rawVersions { + stripped := strings.TrimPrefix(v, "v") + instances = append(instances, &E2EInstance{ + URL: fmt.Sprintf("https://%s.test.example.com", stripped), + ServerVersion: stripped, + }) + } + assert.Equal(t, "11.0.1", instances[0].ServerVersion) + assert.Equal(t, "11.1.0", instances[1].ServerVersion) + for _, inst := range instances { + assert.False(t, strings.HasPrefix(inst.ServerVersion, "v"), + "CMT instance ServerVersion must not start with 'v'") + } + }) + + t.Run("CMT matrix JSON contains stripped versions", func(t *testing.T) { + // buildDesktopCMTMatrixJSON uses instance.ServerVersion directly. + // With stripped versions, the matrix has Docker Hub compatible version strings. + versions := []string{"11.0.1", "11.1.0"} // already stripped + instances := []*E2EInstance{ + {URL: "https://11-0-1.example.com", ServerVersion: "11.0.1"}, + {URL: "https://11-1-0.example.com", ServerVersion: "11.1.0"}, + } + jsonStr, err := buildDesktopCMTMatrixJSON(versions, instances) + require.NoError(t, err) + + var matrix map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(jsonStr), &matrix)) + + servers, ok := matrix["server"].([]interface{}) + require.True(t, ok) + require.Len(t, servers, 2) + + for _, srv := range servers { + s := srv.(map[string]interface{}) + ver := s["version"].(string) + assert.False(t, strings.HasPrefix(ver, "v"), + "CMT matrix version %q must not have 'v' prefix", ver) + } + + s0 := servers[0].(map[string]interface{}) + s1 := servers[1].(map[string]interface{}) + assert.Equal(t, "11.0.1", s0["version"]) + assert.Equal(t, "11.1.0", s1["version"]) + }) + + t.Run("CMT versions capped at 5", func(t *testing.T) { + input := "v1.0.0, v2.0.0, v3.0.0, v4.0.0, v5.0.0, v6.0.0, v7.0.0" + parsed := parseServerVersionsFromString(input) + const maxVersions = 5 + if len(parsed) > maxVersions { + parsed = parsed[:maxVersions] + } + assert.Len(t, parsed, 5, "CMT versions must be capped at 5") + }) +} diff --git a/server/e2e_tests.go b/server/e2e_tests.go index 6db2d0f..86060b7 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -187,6 +187,7 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType "platforms": len(platforms), }) + version := s.resolveE2EServerVersion() username := s.Config.E2EUsername password := s.getE2EPassword(instanceType) // Name format: {type}-pr-{pr}-{platform}-{hex6} @@ -214,7 +215,7 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType instanceType, fmt.Sprintf("pr-%d", pr.Number), platform, uid, ) logger.WithField("instance", instanceName).Info("Creating E2E instance") - inst, err := s.createCloudInstallation(ctx, instanceName, s.Config.E2EServerVersion, username, password, instanceType, logger) + inst, err := s.createCloudInstallation(ctx, instanceName, version, username, password, instanceType, logger) if err != nil { cancel() // signal sibling goroutines to stop waiting results[idx] = result{err: err} @@ -496,7 +497,7 @@ func (s *Server) triggerDesktopE2EWorkflow(ctx context.Context, client *github.C "instance_details": instanceDetailsJSON, "version_name": pr.Ref, "MM_TEST_USER_NAME": s.Config.E2EUsername, - "MM_SERVER_VERSION": s.Config.E2EServerVersion, + "MM_SERVER_VERSION": instances[0].ServerVersion, "pr_number": fmt.Sprintf("%d", pr.Number), }, } @@ -716,6 +717,58 @@ func (s *Server) cleanupNonPRE2EInstancesOnStartup() { logger.Info("Startup non-PR E2E instance cleanup complete") } +// resolveE2EServerVersion returns the Mattermost server version to use for E2E instances. +// If E2EServerVersion is "latest", it fetches the mattermost/mattermost GitHub releases, +// skips RC/beta/alpha tags by name, and returns the first (newest) full release tag +// stripped of its "v" prefix (e.g. "v11.6.0" → "11.6.0"), matching the Docker Hub tag format. +func (s *Server) resolveE2EServerVersion() string { + if s.Config.E2EServerVersion != "latest" { + return s.Config.E2EServerVersion + } + + ctx := context.Background() + client := newGithubClient(s.Config.GithubAccessToken) + + // Redirect to a mock server when running tests (githubAPIBase is empty in production). + if s.githubAPIBase != "" { + if baseURL, parseErr := url.Parse(s.githubAPIBase); parseErr == nil { + client.BaseURL = baseURL + } + } + + var releases []struct { + TagName string `json:"tag_name"` + Draft bool `json:"draft"` + } + + req, err := client.NewRequest("GET", "/repos/mattermost/mattermost/releases?per_page=20", nil) + if err != nil { + s.Logger.WithError(err).Warn("[resolveE2EServerVersion] Failed to build request, falling back to master") + return "master" + } + if _, err = client.Do(ctx, req, &releases); err != nil { + s.Logger.WithError(err).Warn("[resolveE2EServerVersion] Failed to fetch releases, falling back to master") + return "master" + } + + for _, r := range releases { + if r.Draft { + continue + } + 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") + s.Logger.WithField("version", version).Info("[resolveE2EServerVersion] Resolved latest Mattermost server version") + return version + } + + s.Logger.Warn("[resolveE2EServerVersion] No stable release found, falling back to master") + return "master" +} + // destroyE2EInstances destroys all given E2E instances func (s *Server) destroyE2EInstances(instances []*E2EInstance, logger logrus.FieldLogger) { for _, instance := range instances { @@ -930,7 +983,7 @@ func (s *Server) dispatchDesktopE2EWorkflow(repoOwner, repoName, ref, sha, insta // Determine the server version to use for the workflow. // Default to the configured E2E server version, but prefer the actual // provisioned version from the instance details when available. - serverVersion := s.Config.E2EServerVersion + serverVersion := s.resolveE2EServerVersion() if instanceDetailsJSON != "" { var instances []struct { ServerVersion string `json:"server_version"` diff --git a/server/pull_request.go b/server/pull_request.go index 68d5bfb..b638e4e 100644 --- a/server/pull_request.go +++ b/server/pull_request.go @@ -65,8 +65,11 @@ func (s *Server) handlePullRequestEvent(event *github.PullRequestEvent) { return } if s.isE2ELabel(label) { - logger.WithField("label", label).Info("PR E2E test label was removed, canceling workflow runs but keeping servers alive") - go s.cancelPRWorkflowRuns(pr, logger) + // Do not cancel in-progress workflow runs on label removal. + // The E2E workflow itself removes the label upon completion, so + // cancelling here would kill the still-finishing run. Human-triggered + // label removals are also left to run to completion. + logger.WithField("label", label).Info("PR E2E test label was removed, keeping workflow runs alive") return } if s.isSpinWickLabel(label) { diff --git a/server/push_events.go b/server/push_events.go index a12f45c..5e35099 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -154,7 +154,7 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, }) // Name format: {type}-{version}-{platform}-{hex6} - serverVersion := s.Config.E2EServerVersion + serverVersion := s.resolveE2EServerVersion() if version != "" { serverVersion = version } diff --git a/server/server.go b/server/server.go index 1322c35..63ba351 100644 --- a/server/server.go +++ b/server/server.go @@ -47,6 +47,11 @@ type Server struct { // "%s-scheduled-%s" (nightly, ends with SHA), "%s-cmt-%d-%s" (CMT, ends with SHA). e2eInstances map[string][]*E2EInstance e2eInstancesLock sync.Mutex + + // githubAPIBase overrides the GitHub API base URL (e.g. "https://api.github.com/"). + // When non-empty (tests only), GitHub clients created inside this server will be + // redirected to this URL instead of the real GitHub API. + githubAPIBase string } const ( diff --git a/server/workflow_run.go b/server/workflow_run.go index db3e5c8..e0425ac 100644 --- a/server/workflow_run.go +++ b/server/workflow_run.go @@ -159,7 +159,7 @@ func (s *Server) handleNightlyE2ETrigger(owner, repoName, branch, sha, triggerEv return } - instances, err := s.createCMTInstancesForVersion(repoName, instanceType, s.Config.E2EServerVersion, "nightly") + instances, err := s.createCMTInstancesForVersion(repoName, instanceType, s.resolveE2EServerVersion(), "nightly") if err != nil { logger.WithError(err).Error("Failed to create nightly E2E instances") return @@ -310,6 +310,8 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, if version == "" { continue } + // Docker Hub tags use bare semver (e.g. "11.6.0"), not "v11.6.0". + version = strings.TrimPrefix(version, "v") logger.WithField("version", version).Info("Creating CMT instance for server version") From 213dde295dcfcb5d85b11e737be8dccb5e8d3ec5 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 15 Apr 2026 11:02:28 +0530 Subject: [PATCH 2/4] fix(e2e): add prerelease check, timeout, and cache to resolveE2EServerVersion - Add `prerelease` boolean field check (GitHub's explicit flag) alongside the existing draft and tag-name pattern filters so releases marked prerelease on GitHub are never selected even when their tag looks stable. - Replace context.Background() with a 10-second timeout to prevent instance-creation goroutines from blocking indefinitely on slow/unavailable GitHub API. - Cache the resolved version in-memory for 1 hour so back-to-back provisioning calls share one API round-trip. Fallback "master" results are intentionally not cached so the next call retries the API. - Add test coverage for all three new behaviours. Co-Authored-By: Claude Sonnet 4.6 --- server/e2e_dryrun_test.go | 80 +++++++++++++++++++++++++++++++++++++++ server/e2e_tests.go | 43 ++++++++++++++++++--- server/server.go | 9 +++++ 3 files changed, 126 insertions(+), 6 deletions(-) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 5689684..7ff3c44 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -979,6 +979,86 @@ func TestDryRun_ResolveE2EServerVersion(t *testing.T) { s := newDryRunServerLatest(t, srv) assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) }) + + t.Run("prerelease flag skipped even when tag name looks stable", func(t *testing.T) { + // GitHub marks v11.6.0 as prerelease=true (unusual but possible). + // The prerelease field must take precedence over the tag name check. + body := `[ + {"tag_name":"v11.6.0","draft":false,"prerelease":true}, + {"tag_name":"v11.5.0","draft":false,"prerelease":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.5.0", s.resolveE2EServerVersion()) + }) + + t.Run("prerelease and rc tag both skipped, stable returned", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.0","draft":false,"prerelease":true}, + {"tag_name":"v11.6.1-rc1","draft":false,"prerelease":false}, + {"tag_name":"v11.6.0","draft":false,"prerelease":false} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServerLatest(t, srv) + assert.Equal(t, "11.6.0", s.resolveE2EServerVersion()) + }) + + t.Run("resolved version is cached — API called only once", func(t *testing.T) { + callCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/releases") { + callCount++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[{"tag_name":"v11.6.0","draft":false,"prerelease":false}]`)) + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + s := newDryRunServerLatest(t, srv) + + v1 := s.resolveE2EServerVersion() + v2 := s.resolveE2EServerVersion() + v3 := s.resolveE2EServerVersion() + + assert.Equal(t, "11.6.0", v1) + assert.Equal(t, "11.6.0", v2) + assert.Equal(t, "11.6.0", v3) + assert.Equal(t, 1, callCount, "GitHub API must be called exactly once; subsequent calls use the cache") + }) + + t.Run("fallback master is not cached — retried on next call", func(t *testing.T) { + callCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/releases") { + callCount++ + // First call fails; second call succeeds. + if callCount == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[{"tag_name":"v11.6.0","draft":false,"prerelease":false}]`)) + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + s := newDryRunServerLatest(t, srv) + + v1 := s.resolveE2EServerVersion() // API error → fallback "master" (not cached) + v2 := s.resolveE2EServerVersion() // retried → "11.6.0" (now cached) + v3 := s.resolveE2EServerVersion() // cache hit → "11.6.0" + + assert.Equal(t, "master", v1, "first call: API error should fall back to master") + assert.Equal(t, "11.6.0", v2, "second call: should retry and resolve correctly") + assert.Equal(t, "11.6.0", v3, "third call: should return cached value") + assert.Equal(t, 2, callCount, "API should be called twice: once for the failed attempt, once for the retry") + }) } // ------------------------------------------------------------ diff --git a/server/e2e_tests.go b/server/e2e_tests.go index 86060b7..ec44b3f 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -719,14 +719,35 @@ func (s *Server) cleanupNonPRE2EInstancesOnStartup() { // resolveE2EServerVersion returns the Mattermost server version to use for E2E instances. // If E2EServerVersion is "latest", it fetches the mattermost/mattermost GitHub releases, -// skips RC/beta/alpha tags by name, and returns the first (newest) full release tag -// stripped of its "v" prefix (e.g. "v11.6.0" → "11.6.0"), matching the Docker Hub tag format. +// 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. func (s *Server) resolveE2EServerVersion() string { if s.Config.E2EServerVersion != "latest" { return s.Config.E2EServerVersion } - ctx := context.Background() + const cacheTTL = 1 * time.Hour + + // Return cached value if still fresh. + s.e2eVersionCacheLock.Lock() + if s.e2eVersionCache != "" && time.Since(s.e2eVersionCacheTime) < cacheTTL { + cached := s.e2eVersionCache + s.e2eVersionCacheLock.Unlock() + s.Logger.WithField("version", cached).Debug("[resolveE2EServerVersion] Returning cached version") + return cached + } + 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). @@ -737,8 +758,9 @@ func (s *Server) resolveE2EServerVersion() string { } var releases []struct { - TagName string `json:"tag_name"` - Draft bool `json:"draft"` + TagName string `json:"tag_name"` + Draft bool `json:"draft"` + Prerelease bool `json:"prerelease"` } req, err := client.NewRequest("GET", "/repos/mattermost/mattermost/releases?per_page=20", nil) @@ -752,9 +774,12 @@ func (s *Server) resolveE2EServerVersion() string { } for _, r := range releases { - if r.Draft { + // 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). lower := strings.ToLower(r.TagName) if strings.Contains(lower, "-rc") || strings.Contains(lower, "-beta") || strings.Contains(lower, "-alpha") { continue @@ -762,6 +787,12 @@ func (s *Server) resolveE2EServerVersion() string { // Strip "v" prefix to match Docker Hub tag format (e.g. "v11.6.0" → "11.6.0"). version := strings.TrimPrefix(r.TagName, "v") s.Logger.WithField("version", version).Info("[resolveE2EServerVersion] Resolved latest Mattermost server version") + + s.e2eVersionCacheLock.Lock() + s.e2eVersionCache = version + s.e2eVersionCacheTime = time.Now() + s.e2eVersionCacheLock.Unlock() + return version } diff --git a/server/server.go b/server/server.go index 63ba351..c950571 100644 --- a/server/server.go +++ b/server/server.go @@ -52,6 +52,15 @@ type Server struct { // When non-empty (tests only), GitHub clients created inside this server will be // redirected to this URL instead of the real GitHub API. githubAPIBase string + + // e2eVersionCache holds the last successfully resolved "latest" server version so + // that back-to-back E2E provisioning requests (e.g. three parallel platform + // instances) share one GitHub API round-trip instead of each making their own. + // The cache is intentionally short-lived: new stable releases ship at most once a + // month, so a 1-hour TTL gives a good hit rate without risking stale data. + e2eVersionCache string + e2eVersionCacheTime time.Time + e2eVersionCacheLock sync.Mutex } const ( From 86e39990172b6f0f0a82693be5a4a0e5f71654cf Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 15 Apr 2026 11:15:54 +0530 Subject: [PATCH 3/4] test(e2e): drive dispatch assertions through real production functions Replace inline hand-built payload maps in three tests with calls to the actual triggerDesktopE2EWorkflow / triggerMobileE2EWorkflow functions against a mock GitHub server, so assertions validate real dispatch output rather than maps the tests constructed themselves. Add newTestGitHubClient helper that creates a *github.Client whose BaseURL points at a given httptest.Server. CMT instance-construction tests are left as-is: handleCMTWithServerVersions requires a CloudClient that the intentionally-lightweight dry-run suite does not provide. Co-Authored-By: Claude Sonnet 4.6 --- server/e2e_dryrun_test.go | 105 +++++++++++++++++++++++++++----------- 1 file changed, 74 insertions(+), 31 deletions(-) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 7ff3c44..f426f11 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -16,15 +16,18 @@ package server import ( + "context" "encoding/json" "fmt" "io" "net/http" "net/http/httptest" + "net/url" "strings" "sync" "testing" + gogithub "github.com/google/go-github/v32/github" "github.com/mattermost/matterwick/model" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -120,6 +123,18 @@ func newDryRunServer(t *testing.T, apiBase, org string) *Server { } } +// newTestGitHubClient creates a *github.Client whose BaseURL points at srv so that +// production dispatch functions (triggerDesktopE2EWorkflow, triggerMobileE2EWorkflow, …) +// send their HTTP requests to the mock server instead of api.github.com. +func newTestGitHubClient(t *testing.T, srv *httptest.Server) *gogithub.Client { + t.Helper() + client := gogithub.NewClient(nil) + baseURL, err := url.Parse(srv.URL + "/") + require.NoError(t, err) + client.BaseURL = baseURL + return client +} + // makeDesktopInstances fabricates the 3 desktop instances (linux/macos/windows). func makeDesktopInstances() []*E2EInstance { return []*E2EInstance{ @@ -183,20 +198,30 @@ func TestDryRun_DesktopDispatch(t *testing.T) { }) t.Run("workflow inputs built correctly", func(t *testing.T) { - // MM_SERVER_VERSION must come from instances[0].ServerVersion, NOT from - // s.Config.E2EServerVersion. The instance stores the resolved version set at - // provisioning time, which may differ from config (e.g. config="latest"). - workflowInputs := map[string]interface{}{ - "instance_details": instanceDetailsJSON, - "version_name": "feature-branch", - "MM_TEST_USER_NAME": s.Config.E2EUsername, - "MM_SERVER_VERSION": instances[0].ServerVersion, // NOT s.Config.E2EServerVersion + // Drive the real triggerDesktopE2EWorkflow so assertions validate the + // actual payload produced by production code, not a hand-built map. + ghSrv, captures := mockGitHubServer(t, http.StatusNoContent) + client := newTestGitHubClient(t, ghSrv) + pr := &model.PullRequest{ + RepoOwner: "mattermost", + RepoName: "mattermost-desktop", + Number: 42, + Ref: "feature-branch", + Sha: "abc123", } - assert.Equal(t, instances[0].ServerVersion, workflowInputs["MM_SERVER_VERSION"]) - assert.Equal(t, "e2eadmin", workflowInputs["MM_TEST_USER_NAME"]) - assert.Equal(t, "feature-branch", workflowInputs["version_name"]) - assert.NotEmpty(t, workflowInputs["instance_details"]) + err := s.triggerDesktopE2EWorkflow(context.Background(), client, pr, instances) + require.NoError(t, err) + require.Len(t, *captures, 1) + + c := (*captures)[0] + // MM_SERVER_VERSION must come from instances[0].ServerVersion, NOT from + // s.Config.E2EServerVersion (which may be "latest"). + assert.Equal(t, instances[0].ServerVersion, c.Inputs["MM_SERVER_VERSION"], + "MM_SERVER_VERSION must come from instances[0].ServerVersion, not from config") + assert.Equal(t, s.Config.E2EUsername, c.Inputs["MM_TEST_USER_NAME"]) + assert.Equal(t, pr.Ref, c.Inputs["version_name"]) + assert.NotEmpty(t, c.Inputs["instance_details"]) }) t.Run("workflow path targets e2e-functional.yml", func(t *testing.T) { @@ -1067,8 +1092,8 @@ func TestDryRun_ResolveE2EServerVersion(t *testing.T) { func TestDryRun_MMServerVersionFromInstance(t *testing.T) { t.Run("desktop dispatch uses instances[0].ServerVersion not config", func(t *testing.T) { - // Simulate the case where config says "latest" but the instance was provisioned - // with the resolved version "11.6.0". The dispatch must use the instance value. + // Config says "latest" but instances were provisioned with resolved "11.6.0". + // Drive triggerDesktopE2EWorkflow so we assert the real payload, not a hand-built map. s := newDryRunServer(t, "", "mattermost") s.Config.E2EServerVersion = "latest" // would be wrong if used in dispatch @@ -1085,19 +1110,30 @@ func TestDryRun_MMServerVersionFromInstance(t *testing.T) { ServerVersion: resolvedVersion}, } - // The desktop workflow dispatch builds inputs using instances[0].ServerVersion. - inputs := map[string]interface{}{ - "MM_SERVER_VERSION": instances[0].ServerVersion, + ghSrv, captures := mockGitHubServer(t, http.StatusNoContent) + client := newTestGitHubClient(t, ghSrv) + pr := &model.PullRequest{ + RepoOwner: "mattermost", + RepoName: "mattermost-desktop", + Number: 99, + Ref: "feature-branch", + Sha: "abc123", } - assert.Equal(t, "11.6.0", inputs["MM_SERVER_VERSION"], + err := s.triggerDesktopE2EWorkflow(context.Background(), client, pr, instances) + require.NoError(t, err) + require.Len(t, *captures, 1) + + c := (*captures)[0] + assert.Equal(t, "11.6.0", c.Inputs["MM_SERVER_VERSION"], "MM_SERVER_VERSION must be the resolved instance version, not the 'latest' config sentinel") - assert.NotEqual(t, s.Config.E2EServerVersion, inputs["MM_SERVER_VERSION"], + assert.NotEqual(t, s.Config.E2EServerVersion, c.Inputs["MM_SERVER_VERSION"], "MM_SERVER_VERSION must NOT be the raw config value when config is 'latest'") }) t.Run("mobile dispatch does not include MM_SERVER_VERSION", func(t *testing.T) { - // Mobile workflows receive SITE_1/2/3_URL and PLATFORM — never MM_SERVER_VERSION. + // Drive triggerMobileE2EWorkflow so we assert the real payload, not a hand-built map. + s := newDryRunServer(t, "", "mattermost") instances := []*E2EInstance{ {Name: "inst-site1", Platform: "site-1", URL: "https://site1.test.example.com", InstallationID: "id-1", @@ -1110,21 +1146,28 @@ func TestDryRun_MMServerVersionFromInstance(t *testing.T) { ServerVersion: "11.6.0"}, } - inputs := map[string]interface{}{ - "MOBILE_VERSION": "feature-sha", - "PLATFORM": "both", - "SITE_1_URL": instances[0].URL, - "SITE_2_URL": instances[1].URL, - "SITE_3_URL": instances[2].URL, + ghSrv, captures := mockGitHubServer(t, http.StatusNoContent) + client := newTestGitHubClient(t, ghSrv) + pr := &model.PullRequest{ + RepoOwner: "mattermost", + RepoName: "mattermost-mobile", + Number: 99, + Ref: "feature-branch", + Sha: "feature-sha", } - assert.NotContains(t, inputs, "MM_SERVER_VERSION", + err := s.triggerMobileE2EWorkflow(context.Background(), client, pr, instances, "both") + require.NoError(t, err) + require.Len(t, *captures, 1) + + c := (*captures)[0] + assert.NotContains(t, c.Inputs, "MM_SERVER_VERSION", "mobile dispatch must never include MM_SERVER_VERSION") - assert.NotContains(t, inputs, "instance_details", + assert.NotContains(t, c.Inputs, "instance_details", "mobile dispatch must never include instance_details") - assert.Equal(t, "https://site1.test.example.com", inputs["SITE_1_URL"]) - assert.Equal(t, "https://site2.test.example.com", inputs["SITE_2_URL"]) - assert.Equal(t, "https://site3.test.example.com", inputs["SITE_3_URL"]) + assert.Equal(t, "https://site1.test.example.com", c.Inputs["SITE_1_URL"]) + assert.Equal(t, "https://site2.test.example.com", c.Inputs["SITE_2_URL"]) + assert.Equal(t, "https://site3.test.example.com", c.Inputs["SITE_3_URL"]) }) t.Run("all instances in a PR run share the same resolved version", func(t *testing.T) { From 724495f81c446ac438509c3e4e8d3b2c1c9cbebc Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Wed, 15 Apr 2026 11:20:38 +0530 Subject: [PATCH 4/4] test(e2e): register t.Cleanup(srv.Close) in mockGitHubServer mockGitHubServer was leaking the httptest.Server on test completion. Add t.Cleanup(srv.Close) immediately after server creation, consistent with the pattern already used in mockReleasesServer. Co-Authored-By: Claude Sonnet 4.6 --- server/e2e_dryrun_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index f426f11..6b5f488 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -98,6 +98,7 @@ func mockGitHubServer(t *testing.T, status int) (*httptest.Server, *[]dispatchCa w.WriteHeader(status) })) + t.Cleanup(srv.Close) return srv, &captures }