Mattermost-server: Rewrite existing plugin API tests to exclusively test via actual plugin

[esethna]

In app/plugin_api_test.go (and perhaps other files), there are a variety of unit tests that are meant to exercise the [Plugin API|https://github.com/mattermost/mattermost-server/blob/master/plugin/api.go#L16]. Unfortunately, some of them only exercise the internal app.* methods used by the API, and not the actual RPC layer. This means that the API could be effectively broken for a real plugin, despite passing unit tests.

This ticket captures the effort required to rewrite these tests by exercising the RPC layer. Fortunately, there are already examples of this in the same file, in which Go code is embedded as a multi-line string and compiled during the test. The effort then would be to capture the existing coverage using this format instead.

---

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-14559

[levb]

"For help with this ticket, reach out to @crspeller or @levb"

[reflog]

I'll take this one!

[lieut-data]

The original fix above was reverted, since the plugin tests weren't actually being run. Re-queueing this as a help-wanted ticket in case someone wants to take another stab at the issue. Feel free to build off the original PR.

[reflog]

@lieut-data - can you elaborate what didn't work specifically? Pretty sure I tested it well :)

[lieut-data]

Hey @reflog! It was a few months back when reverted, but IIRC, a number of plugins were
silently panicking from this call:

_, ret := hooks.MessageWillBePosted(nil, nil)

after trying to process the nil parameters — but without actually failing the tests.

I think I stumbled across this when testing something that /should/ have been failing but wasn’t. We decided to revert for simplicity during that particular release, but I hadn’t reopened the original GitHub issue.

[reflog]

@lieut-data Oki, if it gets reopened - please assign to me, I'd like to get it fixed

[lieut-data]

Thanks, @reflog! See my original comments in https://github.com/mattermost/mattermost-server/pull/11713.

[reflog]

@lieut-data - I'm working on a fix (it's an easy one), but wanted to point out that the old approach (before my changes) for testing go plugins is vulnerable to the same issue, panics ARE silently ignored.
Take a look at func TestPluginBots(t *testing.T) {, it's a failing test (panic) but it's passing.

[lieut-data]

Thanks for that analysis, @reflog! Agree with your observations -- would it be feasible to detect and fail these panics are part of this renewed effort?

[reflog]

@lieut-data absolutely. It's a rather easy fix, instead of return nil, "" in plugin tests, I'll do return nil, "OK" and test for "OK" string. This way, if the plugin panics, the return value will not be nil, "" and we'll detect the failure right away.

[reflog]

@lieut-data - I've submitted my fix, but please note that I had to disable a part of test_bots_plugin that is causing a segfault because to be honest - I have no idea why it's failing :)
Since that failure is not related to this ticket - I think we should open a new one to trace it

[lieut-data]

Thanks, @reflog :)

[hanzei]

Fixed with https://github.com/mattermost/mattermost-server/pull/13302

[lieut-data]

Thanks again, @reflog!