Playwright Initial Setup

[panoramix360]

This PR introduces some initial configurations to use this project as a utility for other Plugins in the Mattermost Space.

It adds:

• Testcontainers integration
• Playwright initial configuration
• MattermostContainer that is used by the Plugins to initialize Mattermost to use in E2E tests

As an initial configuration this is already being used by me on another branch inside the mattermost-plugin-todo to create an initial test.

[mickmister]

Awesome work Lucas! 🚀

I have some comments for discussion, including one about putting some example tests in this repo to see this in action! Can't wait to use this awesome functionality 😄

[panoramix360]

Thanks for your comments @mickmister

In the last few commits, I externalized the PluginConfig, added the external plugin configuration, and added the example-test folder to have tests in it.

After reading all of your comments, I think my plan of action will be:

• Analyze thoughtfully the code in mmcontainer.ts and remove everything that it's plugin specific related to make it more generic
• Create and reorganize the plugincontainer.ts to be plugin agnostic
• Check to see if the containers are cleaned after a test failure
• Address the comments to change code in specific places
• Add ESLint to the project and maybe even Prettier to code format (@mickmister can I borrow that from another project? If so, can you point me in the right direction? I can probably search for it as well.)

I'm thinking that maybe the mmcontainer.ts could be called mattermost_container.ts and this will be responsible for:

• Having code that interacts with the testcontainers library
• Have generic code that interacts with the mattermost container in ways that it makes sense for Mattermost functions (like creating new users, creating new channels etc)
• Be plugin-agnostic so we can use that on other tests as well, and not only on plugins.

And the plugincontainer.ts can be renamed to run_plugin_container.ts and this will be responsible for:

• Have a method called RunPluginContainercode that leverages a Mattermost container using the mattermost_container.ts to create a Mattermost instance with a plugin/plugins already installed
• Have utilities that work for plugins and let the dev being able to test the plugin in this way
• Be plugin-agnostic but have a customizable way to interact with the plugin

Is this a good plan?

What do you think?

cc: @mickmister @jespino

Thank you!

[mickmister]

@panoramix360 For eslint, this file from the GitHub plugin is probably good minus the React stuff https://github.com/mattermost/mattermost-plugin-github/blob/master/webapp/.eslintrc.json. I personally don't recommend adding prettier, mainly because eslint has been made standard in our projects with no use of prettier. eslint does everything we have deemed that we need in terms of formatting, even if eslint's primary use case is not for formatting.

I'm thinking that maybe the mmcontainer.ts could be called mattermost_container.ts and this will be responsible for:

• Having code that interacts with the testcontainers library
• Have generic code that interacts with the mattermost container in ways that it makes sense for Mattermost functions (like creating new users, creating new channels etc)
• Be plugin-agnostic so we can use that on other tests as well, and not only on plugins.

This all sounds good 👍

And the plugincontainer.ts can be renamed to run_plugin_container.ts and this will be responsible for:

• Have a method called RunPluginContainercode that leverages a Mattermost container using the mattermost_container.ts to create a Mattermost instance with a plugin/plugins already installed
• Have utilities that work for plugins and let the dev being able to test the plugin in this way
• Be plugin-agnostic but have a customizable way to interact with the plugin

For the terminology here, this makes me think that we are running the plugin in a separate docker container, which is obv not what's happening. Would the developer still have access to the main MattermostContainer? Would the plugin one subclass this, or simply have a reference to it? Maybe we can have a withPlugin method or similar on MattermostContainer that somehow returns or provides the reference to the plugin helper object. Or maybe that plugin class can accept a MattermostContainer as an argument to its constructor.

[panoramix360]

Thanks for confirming it, I think it's easier to work with a path clearer forward 😄

@panoramix360 For eslint, this file from the GitHub plugin is probably good minus the React stuff https://github.com/mattermost/mattermost-plugin-github/blob/master/webapp/.eslintrc.json. I personally don't recommend adding prettier, mainly because eslint has been made standard in our projects with no use of prettier. eslint does everything we have deemed that we need in terms of formatting, even if eslint's primary use case is not for formatting.

Ok, no Prettier then! I noticed that when I worked on the mattermost-mobile project. But let me ask you one question, using just ESLint, it's possible to configure VS Code to format automatically on save? or do we need to run the fix command?

For the terminology here, this makes me think that we are running the plugin in a separate docker container, which is obv not what's happening. Would the developer still have access to the main MattermostContainer? Would the plugin one subclass this, or simply have a reference to it? Maybe we can have a withPlugin method or similar on MattermostContainer that somehow returns or provides the reference to the plugin helper object. Or maybe that plugin class can accept a MattermostContainer as an argument to its constructor.

I agree with that approach, I'll align my code to that. I just was going to use mostly of the structure the code that was already in place regarding the RunContainer since this was the way it was done before.

But I agree that maybe it's better for the dev to interact just with the MattermostContainer itself and get an instance of the plugin and interact with it how it sees fit.

This week I'm trying to organize the code and follow the approaches that we discussed, thanks for the insights!

[panoramix360]

Added the ESLInt configuration!

I'll continue with the other implementations.

[panoramix360]

@mickmister

done! can you take a look at the new structure?

I think it solves a lot of scaling problems within the code, and separates well the MattermostPlugin vs MattermostContainer

I liked the new structure, probably we will have some things to still adjust and enhance regarding other topics, but as a base, I think this is beginning to be more solid.

The tests are passing already. What more tests do you think we could create inside the demo-plugin?

Please share your thoughts so I can improve this if needed.

[mickmister]

What more tests do you think we could create inside the demo-plugin?

@DHaussermann Can you please provide some guidance on where we can find the documented test cases for the demo plugin? @panoramix360 We can pick 1 or 2 of the simplest cases to implement here

it's possible to configure VS Code to format automatically on save? or do we need to run the fix command?

Yep it's a configurable setting for the eslint plugin https://www.digitalocean.com/community/tutorials/workflow-auto-eslinting

I typically type Cmd Shift P, Type eslint, Enter because I don't like the editor changing my stuff while I'm still typing haha. I hit Cmd S quite often 🙂

[mickmister]

Hi @panoramix360, this is looking great! 🚀
I have a few more comments on the PR. Let me know what you think 😄

Thanks so much @panoramix360!

[mickmister]

Maybe we can do something like this to make sure we only do this when we need to. Some versions of node already have this, and if the developer wants to use that version of fetch, then we shouldn't overwrite that version

Suggested change

	import fetch from 'node-fetch';
	global.fetch = fetch;
	import fetch from 'node-fetch';
	if (typeof fetch === 'undefined') {
	global.fetch = fetch;
	}

[panoramix360]

ESLint is complaining:

Is this code correct? 😄

If fetch is undefined, probably we don't want to use it to replace global?

Since the fetch reference here is from the import.

[panoramix360]

@mickmister any thoughts?

[mickmister]

Maybe try this:

if (typeof fetch === 'undefined') {
    (global as any).fetch = fetch;
}

[mickmister]

What more tests do you think we could create inside the demo-plugin?

@panoramix360 Maybe we can run one of the /dialog commands (even just /dialog which shows a dialog with all of the dialog element types), fill out the form, and submit. And then verify the created ephemeral message. This is also good so we can share some utility functions in the library like filling out interactive dialogs

[mickmister]

FYI there is some existing code in the GitHub plugin's e2e tests that works with interactive dialogs that we can share here

[mickmister]

This looking awesome @panoramix360!!

I'll leave to you to decide if we need to implement the demo plugin tests. Maybe we can at least verify that the /dialog slash command exists in the command autocomplete to show that the demo plugin was correctly installed.

One thing I want to be sure of before merging (or maybe after this PR) is that this is usable from external repos. Do you mind trying to import this code from a repo like demo plugin or GitHub plugin e2e tests? You and I agreed that we can keep this as Typescript and do not need to compile it, but maybe we should via a postinstall script in this repo. I'd like to importing with its current state first and see if we run into any issues. We would install the npm dependency as a git-based dependency right now since we don't have this published on npm.

Great work Lucas!!

[panoramix360]

hey @mickmister! thanks for the feedback!

I worked on some minor details and now it's just missing the /dialog tests, I'll try to do it in these next few days so we can merge this PR.

Thank you!

[panoramix360]

@mickmister

I added a test example after some effort, I ran into some problems but already solved talking through the community, thank you for that

You can already review the latest changes.

I'll work on running this on the CI now, let's see if it's easy enough.

[panoramix360]

hey @mickmister

The CI is configured and passing already, really happy with it.

It's taking shape!

Do you think it's good to merge?

[mickmister]

I think this is good to go. Let's ask @jespino for another review then I think we can merge 👍 🚀

[mickmister]

I wonder if it makes sense to have this workflow be in a centralized GH action for other plugin projects to use. The one thing is that the way we set up our shared actions, the caller cannot pass any arbitrary environment variables. I think that works fine since pretty much all configuration is done in the actual test now due to using testcontainers 🎉 🎉

Made a ticket for creating a centralized action/workflow to run playwright tests #6

[panoramix360]

That makes sense! let's work on that in the new ticket!

[mickmister]

Keeping this conversation unresolved to surface the discussion

[mickmister]

Maybe try this:

if (typeof fetch === 'undefined') {
    (global as any).fetch = fetch;
}