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

WIP: Playwright tests with shared repo #713

Closed
wants to merge 4 commits into from

Conversation

mickmister
Copy link
Contributor

Summary

Ticket Link

@mickmister mickmister requested a review from hanzei as a code owner November 17, 2023 05:08
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 15.77%. Comparing base (8f6640b) to head (2b2c6a0).
Report is 29 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #713   +/-   ##
=======================================
  Coverage   15.77%   15.77%           
=======================================
  Files          15       15           
  Lines        5577     5577           
=======================================
  Hits          880      880           
  Misses       4655     4655           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Some of the comments in mattermost-community/mattermost-plugin-todo#236 (review) apply here as well. Let's discuss and merge the PR first.

Comment on lines +114 to +115
# cache: "npm"
# cache-dependency-path: webapp/package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this get removed?

Comment on lines +140 to +142
- name: ci/checkout-mattermost-monorepo
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.

I wonder if the version should be opt-in here. Otherwise changes on mattermost could break the E2E tests in this repo. On the other hand, it might be good if we notice a breaking change early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanzei This is intentional to have it always use master, though I think we could have two workflows if we also want a pinned version, like we did with the Apps plugin.

could break the E2E tests in this repo

The main thing that was breaking things in the plugin e2e tests related to the monorepo where the Playwright helpers that the e2e tests were using, like getLastPost. The function implementations didn't change (because the general DOM structure of the webapp didn't change), but where the function is located can change at any time. These are also not necessarily meant to be consumed by other projects.

If the plugin e2e test fails because a behavior change in the webapp's functionality, I think we would want to see the test failure to see if it's something we need to address. If this does cause an issue, we'll handle it on a case-by-case basis.

To solve the issue of the helpers changing paths etc., the plan is to have these helpers intended to be used by plugin e2e tests defined in a separate repo, currently housed at mattermost-community/mattermost-plugin-e2e-test-utils#1

Copy link
Contributor

Choose a reason for hiding this comment

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

This is intentional to have it always use master, though I think we could have two workflows if we also want a pinned version, like we did with the Apps plugin.

👍 for using the same pattern as the Apps plugin (Release on PR, master on master)

await c.postMessage('/github me');
await c.sendMessage();
await postMessage('/github me', page);
// await c.sendMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Nov 17, 2023
@mickmister mickmister marked this pull request as draft November 20, 2023 02:06
@mickmister mickmister changed the title Playwright tests with shared repo WIP: Playwright tests with shared repo Nov 20, 2023
@mickmister
Copy link
Contributor Author

@hanzei Note that this PR and mattermost-community/mattermost-plugin-todo#236 (which is also using the mattermost-plugin-e2e-test-utils) repo are both WIP and proof of concept atm for sharing code. I've set them both as drafts and added WIP to the PR titles to signal this

@hanzei hanzei added Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a core committer labels Apr 30, 2024
@mickmister
Copy link
Contributor Author

Closing as we're going with a different approach mattermost-community/mattermost-plugin-e2e-test-utils#3

@mickmister mickmister closed this May 22, 2024
@mattermost-build mattermost-build removed the Work In Progress Not yet ready for review label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants