Open panoramix360 opened 7 months ago
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:
mmcontainer.ts
and remove everything that it's plugin specific related to make it more genericplugincontainer.ts
to be plugin agnosticI'm thinking that maybe the mmcontainer.ts
could be called mattermost_container.ts
and this will be responsible for:
testcontainers
libraryAnd the plugincontainer.ts
can be renamed to run_plugin_container.ts
and this will be responsible for:
RunPluginContainer
code that leverages a Mattermost container using the mattermost_container.ts
to create a Mattermost instance with a plugin/plugins already installedIs this a good plan?
What do you think?
cc: @mickmister @jespino
Thank you!
@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 calledmattermost_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 :+1:
And the
plugincontainer.ts
can be renamed torun_plugin_container.ts
and this will be responsible for:
- Have a method called
RunPluginContainer
code that leverages a Mattermost container using themattermost_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.
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!
Added the ESLInt configuration!
I'll continue with the other implementations.
@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.
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 🙂
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
FYI there is some existing code in the GitHub plugin's e2e tests that works with interactive dialogs that we can share here
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!
@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.
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?
This PR introduces some initial configurations to use this project as a utility for other Plugins in the Mattermost Space.
It adds:
As an initial configuration this is already being used by me on another branch inside the
mattermost-plugin-todo
to create an initial test.