Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(e2e): playwright base setup #208

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

saturninoabril
Copy link
Member

@saturninoabril saturninoabril commented Sep 19, 2024

Summary

This is mainly based from the awesome works by @rahulsuresh-git and @mickmister at mattermost-community/mattermost-plugin-todo#228. Thanks to both of you!

This plugin template is used to get started in developing plugins for Mattermost, and to make it easier for the developers to write E2E tests, this PR introduces the base setup for Playwright with reference from the mattermost repo (here), taking advantage of the patterns/models/setup readily available from there.

Screenshot 2024-09-19 at 6 46 58 PM

@saturninoabril saturninoabril added the 2: Dev Review Requires review by a core committer label Sep 20, 2024
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @saturninoabril 🚀

Looking forward to more plugins with E2E tests!


- name: ci/checkout-mattermost
run: |
git clone https://github.com/mattermost/mattermost.git ../mattermost
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use --depth here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll use the actions/checkout for consistency with the default depth and branch.

env:
CI: true
NODE_OPTIONS: '--no-experimental-fetch'
PW_ENSURE_PLUGINS_INSTALLED: com.mattermost.plugin-starter-template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we extract the plugin ID programmatically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to do this manually. I'll add notes in addition to what files to edit in the README.

@@ -0,0 +1 @@
20.11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The webapp code of the plugin still used node 16. (https://github.com/mattermost/mattermost-plugin-starter-template/blob/master/.nvmrc#L1)

I'm hesitant that the use of different node versions will cause pain for developers. Can we align the two versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to upgrade this matching the one being used at monorepo's webapp. However, I've seen issues in upgrading webapp's node version here and so it would be good to address separately.

Note that the Playwright from the monorepo uses a feature (native fetch) available from 18+ and so, use of outdated node 16 will not work.

I'll take a look at the options here.

"printWidth": 120,
"singleQuote": true,
"tabWidth": 4
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5 should we use a global confirmation for all webapp node and not only the e2e code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sounds good.

@@ -0,0 +1 @@
export const starterTemplatePluginId = 'com.mattermost.plugin-starter-template';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here. Can we extract the plugin ID?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same -- to do it manually with guides at the readme. This is base test only and will fail if not change.

await systemConsolePage.page.getByRole('link', {name: 'Plugin Starter Template'}).click();

// # Enable the plugin
await page.getByTestId('PluginSettings.PluginStates.com+mattermost+plugin-starter-template.Enabletrue').check();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

.github/workflows/e2e.yml Outdated Show resolved Hide resolved
e2e/README.md Show resolved Hide resolved
#!/bin/sh
echo "Clone mattermost"
cd ../../
git clone --depth 1 https://github.com/mattermost/mattermost.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a plugin dev runs the script a second time? Does git pull the latest changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will fail in cloning the repo (remain as is and will not pull the latest) with the following log but will continue until the end.

fatal: destination path 'mattermost' already exists and is not an empty directory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the script runs without an error and pulls the latest changes on a second run.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but some may prefer to avoid introducing unintended changes to the work they're currently doing in the monorepo. This script is intended solely for the initial setup to help users get started quickly. For subsequent steps, they can choose how they want to update their setup once they're more familiar with the dependencies.

if: success() || failure()
with:
name: test-results
path: e2e/results
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move this as a reusable workflow at https://github.com/mattermost/actions-workflows. With that, I'm putting reviews on hold for now.

@saturninoabril saturninoabril added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core committer labels Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants