Add testcontainers-based integration test infrastructure#241
Conversation
…ture that can be used with any plugin.
Add a reusable testhelper package that spins up real Postgres + Mattermost containers via testcontainers-go, deploys the plugin under test, and provides an authenticated Client4 for exercising the plugin through the Mattermost API. Each call to Setup() resets the database, restarts the Mattermost container (so default roles/permissions are re-initialized), re-deploys the plugin, and creates a fresh team, user, and channel for full test isolation. Key changes: - server/testhelper/ package: container lifecycle, TestHelper, data factories - server/integration_test.go: plugin activation and HTTP endpoint tests - Makefile: test targets depend on dist (single-arch build via BUILD_FOR_TEST) - go.mod: add testcontainers-go v0.40.0
The cd server command persisted across chained commands in the single shell invocation, causing subsequent cd server calls to fail looking for server/server. Fix by cd-ing once and chaining builds with &&.
- Check error return from resp.Body.Close and f.Close - Suppress gosec G304 for os.Open/ReadFile on plugin bundle path (path comes from filepath.Glob, not user input) - Lowercase error string per staticcheck ST1005 - Use 0750/0600 permissions per gosec G301/G306
|
Should we be opinionated and put this into a package importable by all plugins? Or if we are going the route of updating the plugins with AI so having duplicates it's not a problem anymore, maybe putting this outside the server package may help. We still need to remember to commit changes we made to things like this to upstream. |
Thanks @fmartingr I put it here for discoverability and based on the knowledge that we will have a way to keep plugins updated with starter-template changes. Technically it is importable where it is, although it would be confusing to import a package you already have. If we want to put this somewhere it can be imported, then we should make the whole starter template just a stub that imports all the rest. |
I don't mind the code in the starter-template, just split between the |
@fmartingr I like that this test tooling is completely contained in a package and not sprinkled around. It's in the server package because it only applies to server unit tests. I'm open to putting it somewhere else, but if we do, we should bring all the plugin tooling from the starter-template with it so we can update easily. |
hanzei
left a comment
hanzei
left a comment
There was a problem hiding this comment.
we should bring all the plugin tooling from the starter-template with it so we can update easily.
That is something I would love to do moving forward. But some non-trival challenges with the various tools @fmartingr and I tested.
This new library can fully live in a shared place without interfering with existing plugins. We did experience pain in the past by not sharing common tools among plugins. I suggest we learn from that and do it right from the start this time.
There is a lot of boiler-plate code that can be put in a separate repo and imported into plugins without any special tools. Alternatively we can leave it in the starter template and provide a way to update starter template code, which is miles more complicated. I have a need for this right now and have imported this PR commit into my plugins, which is not ideal. @hanzei @fmartingr if you have a concrete proposal we can implement it, otherwise we can leave it as is in the PR and change it later. |
Once this is merged, would you be happy to import the starter-template into other plugins? I don't mind having to import the starter template to get toolings (we could to the same for the manifest and pluginctl binaries, among other things). This could also live under a |
@fmartingr it is not ideal but until a centralized plugin SDK with upgrade capabilities is created this will do. |
|
@wiggin77 I've created mattermost/mattermost#35643. You can start using it right away. Once the PR is merged, I can create a new release of |
|
Removed myself from reviewers while the discussion of the location of this feature is ongoing. Happy to test and review it on its final place. |
hanzei
left a comment
hanzei
left a comment
There was a problem hiding this comment.
While I would still appreciate if the test helper would live in the public module, it's not a blocker for this PR.
A few questions below
There was a problem hiding this comment.
We want to remove this before merge, right?
There was a problem hiding this comment.
Some projects are keeping plans and others don't. I've moved it to /docs/.
| test: apply webapp/node_modules install-go-tools | ||
| test: BUILD_FOR_TEST = 1 | ||
| test: dist install-go-tools |
There was a problem hiding this comment.
Why where apply webapp/node_modules removed?
There was a problem hiding this comment.
webapp/node_modules is applied via dist -> webapp.
| ## Checks the code style, tests, builds and bundles the plugin. | ||
| .PHONY: all | ||
| all: check-style test dist | ||
| all: check-style test |
There was a problem hiding this comment.
Now that test depends on dist, having dist here too would just build it a second time.
|
And sorry for the delay @wiggin77 🙏 |
|
@fmartingr please weigh in on where this library should live. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR updates Makefile and go.mod wiring for test-mode builds, adds Testcontainers-backed Mattermost/Postgres integration helpers and tests, and adds documentation for the Testcontainers plan plus a separate reusable-package extraction plan. ChangesMattermost integration testing
Reusable package extraction plan
Sequence Diagram(s)sequenceDiagram
participant Setup
participant ensureContainers
participant resetDatabase
participant deployPlugin
participant mmctl
participant MattermostServer as "Mattermost server"
Setup->>ensureContainers: start shared Postgres and Mattermost containers
ensureContainers->>mmctl: create admin user with --local
Setup->>resetDatabase: reset database and restart Mattermost
resetDatabase->>MattermostServer: mattermost db reset --confirm
resetDatabase->>mmctl: recreate admin user
Setup->>deployPlugin: upload bundle and enable plugin
deployPlugin->>MattermostServer: poll GetPluginStatuses until PluginStateRunning
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)docs/TESTCONTAINER_PLAN.mdmarkdownlint-cli2 wrapper config was not available before execution docs/plan-extract-reusable-packages.mdmarkdownlint-cli2 wrapper config was not available before execution Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
server/testhelper/helper.go (1)
92-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShared container reset makes
Setupunsafe undert.Parallel().
ensureContainersreturns a single shared container, and eachSetupcall truncates the DB and restarts it. If a plugin author marks any test using this helper witht.Parallel(), concurrentSetupcalls will clobber each other's state and produce flaky, hard-to-debug failures. The current tests are sequential so this is not an active failure, but since this helper is meant to be reused, consider documenting the constraint (or guarding it).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/testhelper/helper.go` around lines 92 - 103, The Setup helper currently resets a shared container, so concurrent use with t.Parallel() can race and wipe each other’s state. Update helper.Setup to either document that it must not be used from parallel tests or add a guard/serialization around the shared reset path, especially where ensureContainers and resetDatabase are called. Keep the constraint clear in the helper so plugin tests don’t accidentally introduce flaky parallel behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Around line 10-11: The dependency set in go.mod pulls in an unfenced Docker
client through github.com/testcontainers/testcontainers-go via
server/testhelper, so update the testcontainers-go version to a release that
uses a patched github.com/docker/docker or add a replacement/constraint to fence
that transitive path. Make the change in the module requirements around
github.com/testcontainers/testcontainers-go and its postgres module so the
resolved Docker client is no longer v28.5.1+incompatible.
In `@server/integration_test.go`:
- Around line 26-28: Add per-call deadlines to the integration-test requests so
they can’t hang indefinitely. In TestPluginActivation, replace the
context.Background() used with AdminClient.GetPluginStatuses with a context
created via context.WithTimeout and cancel it after the call. In
TestHelloEndpoint, build the HTTP request with http.NewRequestWithContext (or
use a client timeout) so the request has its own timeout; use the existing test
helper identifiers like TestPluginActivation, TestHelloEndpoint, and
GetPluginStatuses to locate the changes.
In `@server/testhelper/helper.go`:
- Around line 229-240: Update the CreateUser doc comment in TestHelper so it
refers to adminPassword instead of hardcoding the literal password string; keep
the comment aligned with the CreateUser function’s Password field usage and
avoid duplicating the value in prose so it cannot drift from the adminPassword
constant.
In `@server/testhelper/mmcontainer.go`:
- Around line 38-42: The shared mmcontainer setup/reset path is racy under
concurrent Setup() calls because sharedContainers, resetDatabase, and serverURL
are mutated globally without synchronization; update mmContainers.Setup and the
reset/stop-start flow to serialize all shared-state changes, or add a guard that
fails fast when the helper is invoked from parallel tests. Use the existing
mmContainers, Setup, and resetDatabase symbols to locate the shared state and
ensure all mutations to serverURL and container lifecycle happen under one lock
or single-flight path.
- Around line 57-158: The container lifecycle code in mmcontainerSetup and
resetDatabase is using context.Background(), so a stuck Docker/network/container
call can hang tests indefinitely. Introduce a timeout-backed context in the
container setup path around network.New, postgres.Run,
testcontainers.GenericContainer, mmC.Host, mmC.MappedPort, and execInContainer,
and thread that same bounded context through resetDatabase as well. Also update
the cleanup paths in mmContainers methods and helper calls that Stop and Start
containers so they use the timeout context rather than an unbounded background
context.
- Around line 18-20: The default Mattermost test image in mmcontainer.go is
still using a floating latest tag, which makes test behavior drift unexpectedly.
Update the defaultMMImage constant to a concrete release tag while keeping
MM_TEST_IMAGE as the override path in the helper logic, and make sure any helper
docs or plan text that references the default image are updated to match the
pinned version. Use the existing defaultMMImage and related container setup code
to locate the change.
In `@server/testhelper/TESTCONTAINER_PLAN.md`:
- Around line 54-56: The teardown guidance is inconsistent with the actual
shared-container lifecycle in mmcontainer.go: remove the claim that the first
Setup() registers testcontainers.CleanupContainer via t.Cleanup(), and update
the teardown section to describe the implemented behavior for shared containers
and Ryuk accurately. Refer to the Setup and mmcontainer shared-container flow,
and ensure the documentation matches that explicit t.Cleanup() is not used for
these containers.
- Around line 188-195: The testcontainers dependency versions in
TESTCONTAINER_PLAN should match the versions pinned in go.mod. Update the
references for github.com/testcontainers/testcontainers-go and
github.com/testcontainers/testcontainers-go/modules/postgres from the outdated
v0.35.0 values to the current v0.40.0, or remove the fixed versions entirely if
the plan is meant to stay version-agnostic. Use the dependency list section in
TESTCONTAINER_PLAN as the place to align these symbols.
---
Nitpick comments:
In `@server/testhelper/helper.go`:
- Around line 92-103: The Setup helper currently resets a shared container, so
concurrent use with t.Parallel() can race and wipe each other’s state. Update
helper.Setup to either document that it must not be used from parallel tests or
add a guard/serialization around the shared reset path, especially where
ensureContainers and resetDatabase are called. Keep the constraint clear in the
helper so plugin tests don’t accidentally introduce flaky parallel behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25b84eba-257c-45b7-8052-2b7d6cb8fe6b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefilego.modserver/integration_test.goserver/testhelper/TESTCONTAINER_PLAN.mdserver/testhelper/doc.goserver/testhelper/helper.goserver/testhelper/helper_test.goserver/testhelper/integration_test.goserver/testhelper/mmcontainer.goserver/testhelper/mmcontainer_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/plan-extract-reusable-packages.md (1)
28-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
Setupdeadline-aware.
Setup()starts real containers, but there is no cancellation or timeout path. If Docker pulls stall or the daemon is unhealthy, the whole suite can hang. Thread acontext.Contextthrough the helper or derive one fromt.Deadline()so startup fails fast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plan-extract-reusable-packages.md` around lines 28 - 35, Make Setup deadline-aware by threading cancellation/timeout through the startup path so container setup cannot hang indefinitely. Update the Setup helper and its related option flow (including any helper it calls to start containers, reset the DB, deploy the plugin, or create test data) to derive and use a context from t.Deadline() or accept a context explicitly, and ensure the container/bootstrap operations respect it and fail fast when the deadline is reached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plan-extract-reusable-packages.md`:
- Around line 150-155: The plugin build include logic relies on GOPATH/pkg/mod
and performs a go mod download during Make parse, which can fail when GOMODCACHE
is configured differently and can trigger unwanted network access. Update the
PLUGIN_BUILD_DIR resolution and plugin.mk lookup to use GOMODCACHE instead of
GOPATH-derived paths, and change the download flow so it is not executed at
parse time while still ensuring include can find the installed plugin.mk after
dependency resolution.
- Around line 105-109: The current mmplugin bootstrap runs during variable
parsing via the MMPLUGIN assignment block, making every make invocation perform
side effects and network work. Move the `$(shell $(GO) install ...)` logic out
of the top-level parse-time flow into an explicit target or helper recipe, and
keep `MMPLUGIN` resolution in the variable setup so it only references the
installed binary after the bootstrap step has run.
---
Nitpick comments:
In `@docs/plan-extract-reusable-packages.md`:
- Around line 28-35: Make Setup deadline-aware by threading cancellation/timeout
through the startup path so container setup cannot hang indefinitely. Update the
Setup helper and its related option flow (including any helper it calls to start
containers, reset the DB, deploy the plugin, or create test data) to derive and
use a context from t.Deadline() or accept a context explicitly, and ensure the
container/bootstrap operations respect it and fail fast when the deadline is
reached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a20d06cc-155a-4a80-894b-3951a76b9306
📒 Files selected for processing (3)
docs/TESTCONTAINER_PLAN.mddocs/plan-extract-reusable-packages.mdserver/testhelper/helper.go
✅ Files skipped from review due to trivial changes (1)
- docs/TESTCONTAINER_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- server/testhelper/helper.go
I'd say it make sense to move it where the other plugin sdks resides, |
| cd server && env CGO_ENABLED=0 GOOS=darwin GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-darwin-amd64; | ||
| cd server && env CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-darwin-arm64; | ||
| cd server && env CGO_ENABLED=0 GOOS=windows GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-windows-amd64.exe; | ||
| @if [ "$(BUILD_FOR_TEST)" = "1" ]; then \ |
There was a problem hiding this comment.
What the benefit of this against MM_SERVICESETTINGS_ENABLEDEVELOPER?
| cd server && env CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-darwin-arm64; | ||
| cd server && env CGO_ENABLED=0 GOOS=windows GOARCH=amd64 $(GO) build $(GO_BUILD_FLAGS) $(GO_BUILD_GCFLAGS) -trimpath -o dist/plugin-windows-amd64.exe; | ||
| @if [ "$(BUILD_FOR_TEST)" = "1" ]; then \ | ||
| echo "Building plugin only for linux-$(DEFAULT_GOARCH) for integration tests"; \ |
3 participants
Summary
Plugin authors currently have two options for testing server-side behavior: mock everything via plugintest.API or write E2E/Playwright tests. Mocks are fragile and don't test real behavior, they verify the plugin calls the right methods with the right arguments, but not that the plugin actually works against a real Mattermost server. E2E tests are slow, hard to write, and test browser-to-server rather than plugin-to-server.
This package fills the gap by giving every plugin a way to write Go integration tests that exercise the plugin through the real Mattermost API: KVStore, posts, channels, users, hooks, slash commands. These get tested against an actual server with a real database. Since it lives in the starter template, every new plugin gets this infrastructure for free.
Adds a server/testhelper/ package that spins up real Postgres + Mattermost containers via testcontainers, deploys the plugin, and provides an authenticated model.Client4 for testing against a live server
Each Setup() call resets the database, restarts the Mattermost container (re-initializing default roles/permissions), re-deploys the plugin, and creates a fresh team/user/channel, guaranteeing full isolation between tests
make test now depends on make dist with BUILD_FOR_TEST=1, building only host-arch instead of all 5 platform targets
Includes unit tests for the testhelper's pure functions and integration tests validating the infrastructure itself (container startup, DB reset isolation, user/channel/post creation, plugin deployment)
Ticket Link
NONE