WIP: Playwright with shared repo, compared with master by mickmister · Pull Request #236 · mattermost-community/mattermost-plugin-todo

mickmister · 2023-11-17T03:53:16Z

Summary

This PR is built off of Playwright PR #228, and is pointing to master. This PR uses a submodule defined at mattermost-community/mattermost-plugin-e2e-test-utils#1 to centralize the Playwright bootstrapping code.

PR comparing to open Playwright PR #235

Ticket Link

…t-plugin-todo into feat/adding-playwright-e2e

…suresh-git/mattermost-plugin-todo into feat/adding-playwright-e2e

…t-plugin-todo into feat/adding-playwright-e2e

codecov-commenter · 2023-11-17T03:55:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1afeeb0) 6.42% compared to head (3c24235) 6.42%.

❗ Current head 3c24235 differs from pull request most recent head 7c41fa3. Consider uploading reports for the commit 7c41fa3 to get more accurate results

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #236   +/-   ##
======================================
  Coverage    6.42%   6.42%           
======================================
  Files          11      11           
  Lines        1712    1712           
======================================
  Hits          110     110           
  Misses       1594    1594           
  Partials        8       8

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

hanzei

That is amazing work 🚀 Thank you @mickmister, and an even bigger thanks to @rahulsuresh-git!

Does it make sense to re-use the actions from https://github.com/mattermost/actions? That could help keeping the code DRY.

hanzei · 2023-11-17T07:55:49Z


 env:
  TERM: xterm
  GO_VERSION: 1.19.6


The rest of CI uses go1.21. (mattermost/actions#13) Should the same version be used here?

hanzei · 2023-11-17T07:57:30Z

+          # cache: "npm"
+          # cache-dependency-path: webapp/package-lock.json


Is this needed any longer? Having a cache would be nice.

NVM, I just saw #233.

Note that we were having issues with this block, where the package-lock.json file itself was being cached, so if you pushed a commit that changed that file, CI didn't end up using the new file

hanzei · 2023-11-17T07:58:20Z

+      # - name: ci/checkout-mattermost-monorepo
+      #   uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0
+      #   with:
+      #     path: ../mattermost
+      #     repository: mattermost/mattermost


hanzei · 2023-11-17T08:00:12Z

+[submodule "e2e/playwright/mattermost-plugin-e2e-test-utils"]
+	path = e2e/playwright/mattermost-plugin-e2e-test-utils
+	url = https://github.com/mattermosttest/mattermost-plugin-e2e-test-utils.git


I don't love seeing a submodule getting used here. It adds a lot of complexity. Are there simpler approaches that you considered?

@hanzei Yes the other two options I've considered is referencing through npm, and just duplicating the code. I definitely don't want to go down the route of duplicating the code. Publishing to npm or referencing as a git-based node dependency is probably what we will end up doing.

The submodule approach was the quickest way that required no hassling with npm. Even if I didn't end up running into an issue with npm, I didn't want that to get in the way when trying this out. A git-based npm dependency wouldn't be much different as far as "use this commit", but I agree the submodule is not as turnkey as referencing a git-based dependency

hanzei · 2023-11-17T08:00:44Z

+[submodule "e2e/playwright/mattermost-plugin-e2e-test-utils"]
+	path = e2e/playwright/mattermost-plugin-e2e-test-utils
+	url = https://github.com/mattermosttest/mattermost-plugin-e2e-test-utils.git


Should the repo be under the Mattermost org? mattermosttest is only used for GitHub API testing.

@hanzei Yes this was just the easiest way for me to have it somewhere other than my personal account. Since it is specifically related to Mattermost, and I was directly referencing it in a Mattermost project as a dependency, I sided to put it there for now. It's mainly a proof of concept to test out sharing code, and figuring out exactly which code should be shared etc.

hanzei · 2023-11-17T08:02:16Z

Is there something we can generalize from this file and move into https://github.com/mattermost/actions?

Yes, I want to "get it right" first though. I don't think we should merge two PRs from two different repos before moving this to the actions repo

hanzei · 2023-11-17T08:02:57Z

@@ -1 +1 @@
-16.13.1
+16.20.2


Nit: Missing end of line

rahulsuresh-git · 2023-11-17T23:32:18Z

I shall be addressing the comments in the coming days. Thanks!
cc: @hanzei @mickmister

saturninoabril

Thanks @mickmister! Overall looks good to me. Left few non-blocking comments.

I'll ask for @mvitale1989 review for his inputs.

saturninoabril · 2023-11-22T09:18:19Z

+          - 10080:10080
+          - 10110:10110
+          - 10025:10025
+      elasticsearch:


Do minio and elasticsearch to be used during testing? If not, maybe these can be removed?

saturninoabril · 2023-11-22T09:18:38Z

+
+        ports:
+          - 8065:8065
+          - 4000:4000


Does port 4000 needs to be exposed?

saturninoabril · 2023-11-22T09:20:40Z

+      - name: ci/checkout-mattermost-monorepo
+        run: |
+          git clone https://github.com/mattermost/mattermost.git ../mattermost
+
+      - name: ci/setup-node-for-mattermost-monorepo


Maybe drop monorepo in name? Same in other instances.

mickmister · 2023-11-22T21:00:44Z

@saturninoabril Note that much of the code being used here is in a separate repo mattermost-community/mattermost-plugin-e2e-test-utils#1, currently being referenced through git submodule

mvitale1989 · 2023-11-28T13:35:36Z

Apologies for the late review, I have little experience with git submodules but have no objection in trying out the approach. I see it working fine among the PR status checks.
No additional comments from my side at this point, thanks Michael!

rahulsuresh-git and others added 30 commits November 1, 2023 10:04

setup playwright using docs

c24bd0f

fixed setup commands in setup-environment script

a3871d2

added readme

389c10f

added config for playwright

e52d248

Merge branch 'master' of https://github.com/rahulsuresh-git/mattermos…

c0d4dc3

…t-plugin-todo into feat/adding-playwright-e2e

integrated playwright utils and added test to check available commands

ef99bdd

removed changes from user.ts

cce3e55

fixed TodoBot URL

ab195be

removed a comment

15ea6cb

improvements to message input function

54c3133

fixed issue with cleanUpBotDMs

007367f

added github workflow for playwright

23c6e32

updated workflow with workflow_dispatch

6646dd0

updated node version in .nvmrc

8d22541

updated package-lock.json

8bc4127

updated package-lock.json for node 16.13.1

98e0dc2

updated package-lock.json for node 16.13.1

3a94bd3

Merge branch 'feat/adding-playwright-e2e' of https://github.com/rahul…

63284a8

…suresh-git/mattermost-plugin-todo into feat/adding-playwright-e2e

added python to workflow

721a285

removed python from workflow and removed package-lock.json cache

4be1742

attempts to fix workflow failure

0e9c707

attempts to fix workflow failure

858d67e

attempts to fix workflow failure

67351b4

reverted workflow changes

f48fdb2

reverted workflow changes

745a291

disabled caching in workflow

3ac3aec

Merge branch 'master' of https://github.com/rahulsuresh-git/mattermos…

a798f8d

…t-plugin-todo into feat/adding-playwright-e2e

updated playwright workflow and node version to 16.20.2

ab0fcd9

updated to node 18.17 and updated setup-environment

ae3e6d2

updated workflow with new steps

f848c87

rahulsuresh-git and others added 5 commits November 16, 2023 18:03

updated workflow to run on pull_request

dd496af

remove extract only what's necessary

e3fb677

add submodule

284e154

update commit on submodule

e2460ce

change actions workflow to use submodule

a5094ae

mickmister requested a review from larkox as a code owner November 17, 2023 03:53

mickmister changed the title ~~Playwright with shared repo, compared~~ Playwright with shared repo, compared with master Nov 17, 2023

mickmister mentioned this pull request Nov 17, 2023

WIP: Playwright with shared repo, compared to open Playwright PR #235

Closed

update path to .auth-user.json

c2239ad

mickmister requested review from hanzei and saturninoabril November 17, 2023 04:17

regenerate package-lock.json

3c24235

hanzei requested changes Nov 17, 2023

View reviewed changes

hanzei mentioned this pull request Nov 17, 2023

[GH-124]: Integrating Playwright for E2E tests #228

Merged

hanzei added the 2: Dev Review Requires review by a core committer label Nov 17, 2023

hanzei mentioned this pull request Nov 17, 2023

WIP: Playwright tests with shared repo mattermost/mattermost-plugin-github#713

Closed

mickmister marked this pull request as draft November 20, 2023 02:07

mickmister changed the title ~~Playwright with shared repo, compared with master~~ WIP: Playwright with shared repo, compared with master Nov 20, 2023

saturninoabril approved these changes Nov 22, 2023

View reviewed changes

saturninoabril requested a review from mvitale1989 November 22, 2023 09:26

mickmister mentioned this pull request Nov 22, 2023

Set up dependency caching for playwright GitHub actions workflow #233

Open

mickmister added 4 commits November 22, 2023 15:43

update test utils commit

17bbf7e

Merge branch 'master' into playwright-with-shared-repo

7c41fa3

update webapp deps

8562797

fix e2e test

d0e4119

mickmister mentioned this pull request Jan 26, 2024

Playwright boiler plate mattermost-community/mattermost-plugin-e2e-test-utils#1

Closed

		# cache: "npm"
		# cache-dependency-path: webapp/package-lock.json

		@@ -1 +1 @@
		16.13.1
		16.20.2 No newline at end of file

Conversation

mickmister commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Uh oh!

codecov-commenter commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulsuresh-git commented Nov 17, 2023

Uh oh!

saturninoabril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickmister commented Nov 22, 2023

Uh oh!

mvitale1989 commented Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mickmister commented Nov 17, 2023 •

edited

Loading

codecov-commenter commented Nov 17, 2023 •

edited

Loading