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: add visual tests infra #1047

Merged
merged 35 commits into from
Nov 18, 2023
Merged

test: add visual tests infra #1047

merged 35 commits into from
Nov 18, 2023

Conversation

NasgulNexus
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
Contributor

gravity-ui-bot commented Oct 11, 2023

Preview is ready.

@amje
Copy link
Contributor

amje commented Oct 11, 2023

@NasgulNexus I think doing test for each component variation is not very efficient, e.g. Button has hundreds of props combination Why don't we take screenshots of component stories from storybook preview instead?

Copy link
Contributor

@korvin89 korvin89 left a comment

Choose a reason for hiding this comment

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

I think it will be better to separate unit and snapshot tests. I suggest the next hierarchy:

> Component/
    > __tests__/
    > __snapshot-tests__/
        > {Component}.{case-scope}/
            > snapshots/
            > {Component}.{case-scope}.test.tsx

What do u think about it?

@NasgulNexus
Copy link
Contributor Author

snapshot-tests/

I would suggest this structure, folder ____snapshot-tests__ or __visual-tests__. file name {Component}.{visual or snapshot-tests}.test.tsx - It seems to me that it will be good if the file name has an additional name different from the usual tests.

Now it itself generates the name of the folder where it will put screenshots, depending on the name of the test. If there are several tests in a folder where screenshots are taken, then I would not put them in one folder. What’s a plus is the ability to quickly find the required screenshot and perform any actions with it.

.github/workflows/playwright.yml Outdated Show resolved Hide resolved
env:
CI: 'true'
- name: Upload Playwright playwright report to GitHub Actions Artifacts
if: always()
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it so that the report was always shown and during development it was clear whether new tests appeared or not

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean isnt's if: always() equal to not specifing if statement at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/gravity-ui/uikit/actions/runs/6888720871/job/18738258388#step:6:4712 tested without if it doesn't work.
because the test crashes and then does not perform other tasks. Without if he will do it only when everything went well

package.json Outdated Show resolved Hide resolved
playwright/README.md Outdated Show resolved Hide resolved
## How to write a test

1. You need to select the component for which you want to write tests
2. In the component folder, create a folder `__tests__`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. In the component folder, create a folder `__tests__`
2. Inside the component folder, create the `__tests__` folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Why new name is __snapshots__?

testMatch: '*/__tests__/*.visual.test.tsx',
updateSnapshots: process.env.UPDATE_REQUEST ? 'all' : 'missing',
snapshotPathTemplate:
'{testDir}/{testFileDir}/{testFileName}-snapshots/{arg}{-projectName}-linux{ext}',
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do this in the Jest manner:

Suggested change
'{testDir}/{testFileDir}/{testFileName}-snapshots/{arg}{-projectName}-linux{ext}',
'{testDir}/{testFileDir}/__snapshots__/{arg}{-projectName}-linux{ext}',

playwright/playwright.config.ts Outdated Show resolved Hide resolved
playwright/playwright.config.ts Outdated Show resolved Hide resolved
</head>
<body>
<div id="root" class="g-root g-root_theme_light"></div>
<script type="module" src="./index.tsx"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume styles can be import like so, no scripts needed:

<link rel="stylesheet" href="index.scss" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without index ts, the tests do not work, through this file it mounts the component

Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you can leave the file empty. But in the dock they recommend connecting from index tsx

src/components/Button/__tests__/ButtonHelpers.tsx Outdated Show resolved Hide resolved
@amje
Copy link
Contributor

amje commented Nov 8, 2023

I think it will be better to separate unit and snapshot tests. I suggest the next hierarchy:

> Component/
    > __tests__/
    > __snapshot-tests__/
        > {Component}.{case-scope}/
            > snapshots/
            > {Component}.{case-scope}.test.tsx

What do u think about it?

@korvin89 @NasgulNexus I suggest to keep the Jest structure with .visual. suffix:

> Component/
    > __tests__/
        > {Component}.{case-scope}.visual.test.tsx
    > __snapshots__/
        > {Component}.{case-scope}/

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

package.json Outdated
"playwright": "playwright test --config=playwright/playwright.config.ts",
"playwright:update": "npm run playwright -- -u",
"playwright:docker": "docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:v1.39.0-jammy /bin/bash -c 'npm ci && npx playwright install && npm run playwright'",
"playwright:update:docker": " docker run --rm --network host -v $(pwd):/work/ -w /work/ -it mcr.microsoft.com/playwright:v1.39.0-jammy /bin/bash -c 'npm ci && npx playwright install && npm run playwright:update'"
Copy link
Contributor

Choose a reason for hiding this comment

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

playwright:docker:update

import {MyTestedComponent} from '../MyTestedComponent';
```

5. Writing a test:
Copy link
Contributor

Choose a reason for hiding this comment

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

I've meant get code from 4 and put it into 5. Title In the file to make imports should be removed also

## How to write a test

1. You need to select the component for which you want to write tests
2. In the component folder, create a folder `__tests__`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why new name is __snapshots__?

playwright/playwright.config.ts Outdated Show resolved Hide resolved
playwright/playwright.config.ts Outdated Show resolved Hide resolved
playwright/README.md Outdated Show resolved Hide resolved
playwright/playwright.config.ts Outdated Show resolved Hide resolved
@amje
Copy link
Contributor

amje commented Nov 16, 2023

@NasgulNexus Why in the recent update all screenshots updated and shifted by 1 pixel?

@NasgulNexus
Copy link
Contributor Author

@NasgulNexus Why in the recent update all screenshots updated and shifted by 1 pixel?

for the ci test, I took screenshots locally. And then I took screenshots through the docker image for linux. And because of the font difference, this happened.
Screenshot 2023-11-16 at 14 02 45
Screenshot 2023-11-16 at 14 02 41
Screenshot 2023-11-16 at 14 02 36
For example, I am applying a test run locally. Waiting is the removal of the docker image

@amje amje changed the title Test component test: add visual tests infra Nov 17, 2023
@NasgulNexus NasgulNexus merged commit e80b088 into main Nov 18, 2023
4 checks passed
@NasgulNexus NasgulNexus deleted the test-component branch November 18, 2023 00:56
@amje amje mentioned this pull request Nov 29, 2023
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