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

test(e2e, playwright): added e2e test for /todo help command #238

Conversation

rahulsuresh-git
Copy link
Contributor

Summary

This introduces a Playwright end-to-end (e2e) test to check if the /todo help command functions as expected. We verify the functionality of the various commands available for the Todo plugin.

I've also made a few changes to improve clarity and readability.
Verified that the test passes locally.

image

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1afeeb0) 6.42% compared to head (18e5802) 6.42%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #238   +/-   ##
======================================
  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.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks for awesome addition @rahulsuresh-git. Tests LGTM, just some comments around formatting and naming

e2e/playwright/tests/test.list.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
@mickmister
Copy link
Contributor

@toninis Any ideas why we need to press "Approve and run" on each commit of this forked repo PR?

Workflows for this repo are here https://github.com/mattermost/mattermost-plugin-todo/tree/master/.github/workflows. Not sure which ones are the "2 workflows awaiting approval"

CleanShot 2023-12-03 at 21 17 44

@toninis
Copy link
Contributor

toninis commented Dec 4, 2023

@toninis Any ideas why we need to press "Approve and run" on each commit of this forked repo PR?

Workflows for this repo are here https://github.com/mattermost/mattermost-plugin-todo/tree/master/.github/workflows. Not sure which ones are the "2 workflows awaiting approval"

CleanShot 2023-12-03 at 21 17 44

This is the organisation default behaviour on forked PRs for security reasons @mickmister

@mickmister
Copy link
Contributor

@toninis Is this rule enforcement new? I know there is a "first-time contributor" check that has been enabled for a while, though I'm not used to seeing this for every commit

@toninis
Copy link
Contributor

toninis commented Dec 4, 2023

@toninis Is this rule enforcement new? I know there is a "first-time contributor" check that has been enabled for a while, though I'm not used to seeing this for every commit

mattermost/mattermost#25616
It's always like this if I recall

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Nice work on this @rahulsuresh-git 🎉 Looks really good. I have some comments for discussion on how we should organize the tests as a whole, and want to discuss the assertions we're making on the help command test

e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
Comment on lines 44 to 66
postMessage('/todo help', page);

// # Grab the last post
const post = await getLastPost(page);
const postBody = post.locator('.post-message__text-container');

// * Assert /todo add [message] command is visible
await expect(postBody).toContainText('add [message]');

// * Assert /todo list command is visible
await expect(postBody).toContainText('list');

// * Assert /todo list [listName] command is visible
await expect(postBody).toContainText('list [listName]');

// * Assert /todo pop command is visible
await expect(postBody).toContainText('pop');

// * Assert /todo send [user] [message] command is visible
await expect(postBody).toContainText('send [user] [message]');

// * Assert /todo settings summary [on, off] command is visible
await expect(postBody).toContainText('settings summary [on, off]');
Copy link
Contributor

Choose a reason for hiding this comment

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

@hanzei Curious what you think about this test. It could be a bit brittle since it's checking the output in this much detail. Essentially if anything changes about the help text (other than the order of commands in the output), this test will break.

We would also want to make this test fail if a new command is added to the help text, so maybe we want to count the number of lines somehow?

I think this is a difficult problem to solve, since we mainly want e2e tests to be "this feature generally works correctly", rather than "this feature's output looks exactly like this"

e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Outdated Show resolved Hide resolved
e2e/playwright/tests/test.list.ts Outdated Show resolved Hide resolved
@mickmister mickmister requested a review from hanzei December 6, 2023 02:50
Copy link
Contributor

@mickmister mickmister 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 on this @rahulsuresh-git! I have a few comments more comments on the implementation. I think we can make the tests a bit less brittle by making their assertions less specific, and some suggestions on code organization

Comment on lines +66 to +67
const postBody = lastPost.locator('.post-message__text-container');
return postBody;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes we'll want to parse other things from the post and not just the body. Maybe we can create a separate function getLastPostBody or getLastPostText and have it call getLastPost.

I'm mainly trying to be weary of any changes to the "meaning" of functions that are already in files being shared among the plugin projects. That's one of the main focuses on the e2e framework. This is essentially an externally facing API for other plugins to use.

e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/todo_plugin.spec.ts Show resolved Hide resolved
e2e/playwright/tests/test.list.ts Show resolved Hide resolved
e2e/playwright/tests/test.list.ts Show resolved Hide resolved
@hanzei
Copy link
Contributor

hanzei commented Dec 18, 2023

Removing myself from the list of reviewers until the comments of @mickmister are resolved.

@rahulsuresh-git
Copy link
Contributor Author

Closing this PR as the changes are moved to this single PR: #254.
Note: Comments addressed on the single PR.

@rahulsuresh-git rahulsuresh-git deleted the feat/todo-help-e2e-test branch May 19, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants