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

Add Playwright tests #337

Merged
merged 48 commits into from
Jun 20, 2024
Merged

Conversation

BrtqKr
Copy link
Contributor

@BrtqKr BrtqKr commented Apr 28, 2024

Details

We wanted to reduce number of regressions occurring after the contributions, so I've done a setup for playwright e2e tests for web.

This PR includes:

  • constants restructuring (everything is wired up to the example, so we can keep this consistent)
  • playwright setup for testing
  • basic tests for styling and input-related actions (cut, paste, copy, cursor change etc)
  • CI action for running those tests

Related Issues

Expensify/App#44028

Manual Tests

cd WebExample
yarn test

Linked PRs

Copy link

github-actions bot commented Apr 28, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link
Collaborator

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

Tests run like a butter 🧈 I left some code-related comments

constants.ts Outdated Show resolved Hide resolved
constants.ts Outdated Show resolved Hide resolved
WebExample/tests/styles.spec.ts Outdated Show resolved Hide resolved
WebExample/tests/styles.spec.ts Outdated Show resolved Hide resolved
WebExample/tests/textManipulation.spec.ts Outdated Show resolved Hide resolved
WebExample/tests/input.spec.ts Outdated Show resolved Hide resolved
WebExample/tests/input.spec.ts Outdated Show resolved Hide resolved
WebExample/tests/textManipulation.spec.ts Outdated Show resolved Hide resolved
Comment on lines 133 to 141
if (startNode?.firstChild && endNode?.lastChild) {
const range = new Range();
range.setStart(startNode.firstChild, 2);
range.setEnd(endNode.lastChild, endNode.lastChild.textContent?.length ?? 0);

const selection = window.getSelection();
selection?.removeAllRanges();
selection?.addRange(range);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting cursor position can also be moved to utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we probably won't be able to use it in here, because this would be only a part of the function used inside of page.evaluate and we aren't able to pass functions to the interior, as a param. I'll add this to the utils anyway, in case you're ever in need of using it somewhere else.

@BrtqKr
Copy link
Contributor Author

BrtqKr commented May 9, 2024

I have read the CLA Document and I hereby sign the CLA

@tomekzaw tomekzaw changed the title Add playwright tests Add Playwright tests May 9, 2024
@BrtqKr BrtqKr marked this pull request as ready for review May 9, 2024 19:21
Copy link
Collaborator

@tomekzaw tomekzaw 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 this PR. Overall, the setup looks good. I've left some comments regarding the implementation of the tests themselves.

@@ -0,0 +1,49 @@
name: E2E Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: E2E Test
name: Test web E2E

on:
pull_request:
paths:
- .github/workflows/e2e-test.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename the file and change this line

Suggested change
- .github/workflows/e2e-test.yml
- .github/workflows/web-e2e-test.yml

branches:
- main
paths:
- .github/workflows/e2e-test.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

- WebExample/**

jobs:
e2e_test:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
e2e_test:
test:

working-directory: ./WebExample

concurrency:
group: e2e-test-${{ github.ref }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here


test.describe('markdown content styling', () => {
test('bold', async ({page}) => {
await testMarkdownContentStyle({styleName: 'bold', style: TEST_CONST.MARKDOWN_STYLE_DEFINITIONS.bold.style, page});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of TEST_CONST.MARKDOWN_STYLE_DEFINITIONS. There's nothing wrong with tests containing string literals in multiple copies.

This test should contain "Hello *world*!" string as well as fontWeight: 'bold' somewhere inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in terms of "Hello *world*!" - sure, but I think having styles defined in a constant is the right way to handle this, If you try checking style definition in multiple cases and/or you are using this styling in a library itself, this might get desynchronized, if someone decides to change it at some point

});
});

test('select', async ({page}) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
test('select', async ({page}) => {
test('select all', async ({page}) => {

@@ -16,7 +18,7 @@ export default class InputHistory {

debounceTime: number;

constructor(depth: number, debounceTime = 150) {
constructor(depth: number, debounceTime = TEST_CONST.INPUT_HISTORY_DEBOUNCE_TIME_MS) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Source code files shouldn't rely on test configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initially, those were constants for general use, and the value for this constant was taken from this place. Michał suggested renaming those constants as test constants, so I can either make them standard constants as intended or revert this change and affect test reliability - someone might change it in the future and we might get different results because tests won't have debounce updated.

testConstants.ts Outdated
Comment on lines 5 to 21
const MARKDOWN_STYLE_DEFINITIONS = {
bold: {wrapContent: (content: string) => `*${content}*`, style: 'font-weight: bold;'},
link: {wrapContent: (content: string) => `https://${content}.com`, style: 'color: blue; text-decoration: underline;'},
title: {wrapContent: (content: string) => `# ${content}`, style: 'font-size: 25px; font-weight: bold;'},
code: {wrapContent: (content: string) => `\`${content}\``, style: 'font-family: monospace; font-size: 20px; color: black; background-color: lightgray;'},
codeBlock: {wrapContent: (content: string) => `\`\`\`\n${content}\n\`\`\``, style: 'font-family: monospace; font-size: 20px; color: black; background-color: lightgray;'},
here: {wrapContent: (content: string) => `@${content}`, style: 'color: green; background-color: lime;'},
mentionUser: {wrapContent: (content: string) => `@${content}@swmansion.com`, style: 'color: blue; background-color: cyan;'},
blockquote: {
wrapContent: (content: string) => `> ${content}`,
style: 'border-color: gray; border-width: 6px; margin-left: 6px; padding-left: 6px; border-left-style: solid; display: inline-block; max-width: 100%; box-sizing: border-box;',
},
roomMention: {
wrapContent: (content: string) => `#${content}`,
style: 'color: red; background-color: pink;',
},
} as const satisfies Record<string, MarkdownStyleDefiniton>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

plz remove this

testConstants.ts Outdated
Comment on lines 23 to 25
const EXAMPLE_CONTENT = Object.entries(MARKDOWN_STYLE_DEFINITIONS)
.map(([styleName, style]) => style.wrapContent(styleName))
.join('\n');
Copy link
Collaborator

Choose a reason for hiding this comment

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

please replace this with a regular JS string (or a concatenated array)

@BrtqKr BrtqKr requested review from tomekzaw and Skalakid May 14, 2024 07:30
Copy link
Collaborator

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

LGTM ✨, let's resolve earlier conversations and I think we are ready to go

Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@tomekzaw tomekzaw 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 this PR! ❤️

@tomekzaw tomekzaw merged commit cf15265 into Expensify:main Jun 20, 2024
5 checks passed
@tomekzaw tomekzaw mentioned this pull request Jul 15, 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