Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CE2E fixes, support MacOS #475

Merged
merged 5 commits into from
Jan 22, 2024
Merged

CE2E fixes, support MacOS #475

merged 5 commits into from
Jan 22, 2024

Conversation

lieut-data
Copy link
Member

@lieut-data lieut-data commented Jan 19, 2024 •
edited
Loading

Summary

Support MacOS in the CE2E tests, forward logs from the Mattermost container, and fix a handful of issues.

Ticket Link

None.

@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Jan 19, 2024
lieut-data
lieut-data commented Jan 19, 2024
View reviewed changes
Makefile
@@ -5,10 +5,11 @@ MM_DEBUG ?=
GOPATH ?= $(shell go env GOPATH)
GO_TEST_FLAGS ?= -race
GO_BUILD_FLAGS ?=
GO_BUILD_TAGS ?=
Copy link
Member Author

Choose a reason for hiding this comment

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

We now apply clientMock from the make dist invocation within ce2e/.

Makefile
Comment on lines +11 to +12
DEFAULT_GOOS ?= $(shell go env GOOS)
DEFAULT_GOARCH ?= $(shell go env GOARCH)
Copy link
Member Author

Choose a reason for hiding this comment

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

Allow ce2e/ to override the "default" GOOS so we can explicitly build for linux and amd64, matching the Docker containers.

Makefile
endif
endif

## Builds the server, if it exists, for all supported architectures, unless MM_SERVICESETTINGS_ENABLEDEVELOPER is set.
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for special targets anymore.

build/manifest/main.go
Comment on lines +66 to +68
// Don't use runtime.* directly, as we may target a different archtiecture for testing.
defaultGoOs := os.Getenv("DEFAULT_GOOS")
defaultGoArch := os.Getenv("DEFAULT_GOARCH")
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy whatever the Makefile is doing.

server/testutils/containere2e/containere2e.go
@@ -14,7 +17,25 @@ import (
"github.com/stretchr/testify/require"
)

var buildPluginOnce sync.Once

func buildPlugin(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Build the plugin especially for these tests, and only do it once.

Copy link
Member

Choose a reason for hiding this comment

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

I like this.

server/testutils/containere2e/containere2e.go
mattermost, err := mmcontainer.RunContainer(ctx,
mmcontainer.WithPlugin(filename, "com.mattermost.msteams-sync", pluginConfig),
mmcontainer.WithEnv("MM_MSTEAMSSYNC_MOCK_CLIENT", "true"),
mmcontainer.WithTestingLogConsumer(t),
Copy link
Member Author

Choose a reason for hiding this comment

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

Forward logs from Mattermost to enable debugging.

server/testutils/containere2e/containere2e.go
)
require.NoError(t, err)

// TODO: This won't be required after jespino gets https://github.com/testcontainers/testcontainers-go/pull/2073 merged.
Copy link
Member Author

Choose a reason for hiding this comment

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

I saw your PR, @jespino! This should help us now before we can more easily configure as you have above.

server/testutils/mmcontainer/testscontainer.go
"MM_SERVICESETTINGS_ENABLELOCALMODE": "true",
"MM_PASSWORDSETTINGS_MINIMUMLENGTH": "5",
"MM_PLUGINSETTINGS_ENABLEUPLOADS": "true",
"MM_PLUGINSETTINGS_AUTOMATICPREPACKAGEDPLUGINS": "false",
Copy link
Member Author

Choose a reason for hiding this comment

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

Most prepackaged plugins don't do anything until configured, so speed things up.

jespino
jespino reviewed Jan 21, 2024
View reviewed changes
server/testutils/containere2e/containere2e.go
cmd := exec.Command("make", "-C", "../../", "dist")
cmd.Env = os.Environ()
cmd.Env = append(cmd.Env, "DEFAULT_GOOS=linux")
cmd.Env = append(cmd.Env, "DEFAULT_GOARCH=amd64")
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that the test runners are going to be amd64? I guess it can end up being arm64 too.

Copy link
Member

Choose a reason for hiding this comment

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

You mention about the docker containers, but I guess docker containers in arm processors should use arm architecture, I think we should be building for the current runner architecture and the GOOS linux, but I'm not 100% sure how it works in Mac.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking now that maybe, we can do the plugin compilation here, already inside a testcontainer. Build the container for go, share the filesystem of the current directory, execute the build, and copy over the file outside the container, tear down the container, and use the compiled version. Not needed in this PR, I think we can merge this and think about adding that later.

Copy link
Member Author

Choose a reason for hiding this comment

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

For context, the issue here is matching the architecture of the Mattermost enterprise docker containers -- otherwise the plugin won't run.

Once we start building and supporting Mattermost for arm, we'll probably need to revisit this.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, you are 100% right.

jespino
jespino approved these changes Jan 21, 2024
View reviewed changes
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM, some concerns about the specificity of the Architecture, but if the CI is working, and the local tests for mac and linux are working, I think we are good.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (58c9f69) 43.45% compared to head (ae60ff8) 43.45%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #475   +/-   ##
=======================================
  Coverage   43.45%   43.45%           
=======================================
  Files          21       21           
  Lines        6289     6289           
=======================================
  Hits         2733     2733           
  Misses       3331     3331           
  Partials      225      225

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wiggin77
wiggin77 approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lieut-data lieut-data merged commit 55a01bf into main Jan 22, 2024
7 checks passed
@lieut-data lieut-data deleted the ce2e-fixes branch January 22, 2024 16:26
@lieut-data lieut-data added QA/Review Not Required and removed 2: Dev Review Requires review by a core committer labels Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jespino jespino jespino approved these changes

@wiggin77 wiggin77 wiggin77 approved these changes

Assignees
No one assigned
Labels
QA/Review Not Required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lieut-data @codecov-commenter @jespino @wiggin77