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): store images in webp format #2451

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

test(e2e): store images in webp format #2451

wants to merge 8 commits into from

Conversation

fshovchko
Copy link
Contributor

Closes: #2393

@fshovchko fshovchko added enhancement Improvements and additions to code needs review The PR is waiting for review e2e labels Jun 5, 2024
@fshovchko fshovchko requested review from a team June 5, 2024 17:30
@fshovchko fshovchko self-assigned this Jun 5, 2024
@fshovchko fshovchko requested review from dshovchko, ala-n and yadamskaya and removed request for a team June 5, 2024 17:30
@fshovchko fshovchko requested a review from dshovchko June 6, 2024 16:09
@ala-n
Copy link
Collaborator

ala-n commented Jun 7, 2024

It is definitely an enhancement that we can use, as we will get files that are practically smaller.
However, it does not utilize the possibility of WebP compression. It improves the file size just because PNG compression that sharp uses by default is better compared to the raw PNG output that pupiter gives out of the box. So, we still store snapshots as PNGs, just with compression (a similar result can be achieved by using the sharp wrapper on top of the screenshot function screenshots.push(await sharp(await page.screenshot(options)).png().toBuffer());).

To summarize everything, I'd have to say:

  1. The sharp library is definitely a good start, and we can leave it in the codebase with 100% confidence. (Just add a note in the README that it may require npm install --include=optional sharp -g to work properly in some cases.)

  2. We can definitely transform this PR into a partial solution that provides compression, which is a valuable and valid improvement (just without WebP until it actually affects file format to store).

  3. Regarding Dima's comment above, I'm not sure that lossy compression should be a problem unless it consistently gives us the same result. We are definitely not going to catch some color problems with the image, so unless compress(snapshot) = compress(actual) is valid for approximate similarity, we are fine.

  4. Regarding the original ticket with WebP, here are some notes to move forward:

    • Snapshot storing is a jest part of the process.
    • It happens inside the matching library, specifically at the toMatchImageSnapshot level. If I remember correctly, we can describe to Jest itself how to store a snapshot, so we might not need to override jest-image-snapshot since it utilizes Jest's original snapshotting. But it still should be a little bit tricky thing to do as I mentioned initially.

@ala-n ala-n added under discussion and removed needs review The PR is waiting for review labels Jun 7, 2024
Copy link
Collaborator

@dshovchko dshovchko left a comment

Choose a reason for hiding this comment

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

If compression is ok for us I vote for storing images in AVIF format. AVIF has higher compression efficiency than WebP.

@ala-n
Copy link
Collaborator

ala-n commented Jun 7, 2024

If compression is ok for us, I vote for storing images in AVIF format. AVIF has higher compression efficiency than WebP.

This is a totally valid suggestion. If sharp (or another image processor) can work with AVIF (approximately fast), we can use it. I'll update the original ticket.

@fshovchko fshovchko requested a review from dshovchko June 14, 2024 14:59
Copy link

codeclimate bot commented Jun 18, 2024

Code Climate has analyzed commit 59caec2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 65.3% (0.0% change).

View more on Code Climate.

@fshovchko fshovchko added the needs review The PR is waiting for review label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e enhancement Improvements and additions to code needs review The PR is waiting for review under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[⚡e2e]: ability to store image snapshots as a WebP/AVIF
3 participants