mattermost / mattermost-plugin-msteams

MS Teams plugin for Mattermost
Other
13 stars 11 forks source link

MM-5701: reenable reattached plugin tests #563

Closed lieut-data closed 5 months ago

lieut-data commented 5 months ago

Summary

At a high level, this PR begins our move move away from mocks for the pluginapi -- that means no more fiddling with making tests pass only to realize the mock assertions were broken to begin with. All the rewritten tests now use a real pluginapi and a real store backed by a real Mattermost server and a real Postgres instance. We use @jespino's newly added testcontainers to host the Postgres instance, but run the server in memory meaning we're not dependent on a particular Docker container build.

(@jespino, I wonder if we should migrate our ce2e/ tests in this direction as this provides greater flexibility when testing plugin + server changes. As these have been very flaky lately, I've temporarily disabled them as part of this PR.)

This PR depends the plugin reattach model introduced with https://github.com/mattermost/mattermost/pull/26421, but we don't actually need it to be merged right away, as we can point directly at the commit via go.mod. (Naturally, we'll clean this up once merged!)

This is "part 1" of what I hope to be a series of PRs that evict almost all mocking from our unit tests. Of course, we still mock for the MS Teams Graph API of course, but even that's improved because we no longer panic when the test fails: the errors now get emitted through the testing.T normal interface.

It may be helpful to review the individual commits. I briefly explored putting a bunch of the helper_* code directly into our public package in the monorepo for immediate reuse, but there's some dependencies on server/v8 that make this impossible, and I also wanted some time to soak these changes in practice before promoting them more widely.

Ticket Link

Relates-to: https://mattermost.atlassian.net/browse/MM-56022

Important Notes

This change requires go 1.21 as a result of the updated server dependency. I'll sort out any downstream impacts of this change as we discover them (e.g. if the plugin tooling needs to be updated or not).

lieut-data commented 5 months ago

@cpoile, @fmartingr, @crspeller, @hanzei & @mickmister, same as https://github.com/mattermost/mattermost/pull/26421, I welcome all feedback, but don't actually expect everyone to review. Feel free to remove yourself or review as you see fit -- mostly wanting to maximize the success of this effort.

lieut-data commented 5 months ago

Note that a full suite execution without caching is still tolerably fast -- the overhead of reattaching plugins is negligible. It is slower, but mostly because there's a real pluginapi and database doing work now, so the tradeoff to make the tests "real" is worth it.

CleanShot 2024-03-28 at 09 53 21@2x

lieut-data commented 5 months ago

Thanks, @jespino! Disabling those tests definitely itself isn't needed as part of this PR: could I ask you to push a patch that removes those build flags?

And could you help own the flakiness of those tests in CI? These are ones I've had to re-run repeatedly over the last few weeks, slowing down development. Disabling was a coarse solution, but if we can fix them that would be better :)

jespino commented 5 months ago

@lieut-data no problem, I'll remove that, about making the tests pass, I can take care of it too.

jespino commented 5 months ago

@lieut-data tests are working and main branch is merged :)

lieut-data commented 5 months ago

Thank you, @jespino! I really appreciate that :)