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.

Sign up for GitHub

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

Jump to bottom

Error: "Error running image diff." for large images #210

Closed
lukasoppermann opened this issue May 19, 2020 · 37 comments
Closed

Error: "Error running image diff." for large images #210

lukasoppermann opened this issue May 19, 2020 · 37 comments
Labels
bug

Comments

@lukasoppermann
Copy link

Hey,

for some of my tests I get the error Error running image diff..
As those are the longest pages e.g.  14252px I assume the length is the deciding factor.

Other pages work fine. The diff file was also created and is in the folder.

Can I do something to fix this (e.g. is there a config option I missed)?
@anescobar1991
Copy link
Member

anescobar1991 commented May 26, 2020 •
edited

Not sure, have not seen this one before. Can you try to come up with a test to easily replicate and see if there is something we can do? Likely this is an issue downstream with pixelmatch but we could take a look.

@lukasoppermann
Copy link
Author

Hey, sure here is a test case that replicates the error. I hope that helps. Please let me know if anything in my code does not make sense and I can test if changes there fix it.

const puppeteer = require('puppeteer')
const { toMatchImageSnapshot } = require('jest-image-snapshot')
let browser
let page

  beforeAll(async () => {
    // start Puppeteer with a custom configuration, see above the setup
    browser = await puppeteer.launch({
      ignoreHTTPSErrors: true,
      headless: true,
      args: ['--no-sandbox', '--disable-setuid-sandbox'],
    })
    expect.extend({ toMatchImageSnapshot })
    // load page
    page = await browser.newPage()
    await page.goto(`https://www.theverge.com/`, {waitUntil: 'load'});
    // scroll to bottom and back up
    await page.waitFor(1000)
    await page.evaluate(() => {
      window.scrollTo(0, Number.MAX_SAFE_INTEGER)
    })
    await page.waitFor(500)
    await page.evaluate(() => {
      window.scrollBy(0, 0)
    })
    await page.waitFor(500)
  })

  test(`Testing viewport: 1400`, async () => {
    // set viewport
    await page.setViewport({
      width: 1400,
      height: 900,
      deviceScaleFactor: 1
    })
    // take full screen screenshot
    let image = await page.screenshot({
      path: `${__dirname}/test_snaps/verge-1440.png`,
      type: 'png',
      fullPage: true
    })
    // compare screenshot
    expect(image).toMatchImageSnapshot({
      customDiffConfig: {
        threshold: 0.01
      },
      customDiffDir: `${__dirname}/test_snaps/verge-1440.png`,
      customSnapshotsDir: `${__dirname}/baseline/`,
      customSnapshotIdentifier: `verge-1440`,
      noColors: true
    })
  }, 15000)

  afterAll(async () => {
    await browser.close()
  })

@Kradirhamik
Copy link

Hi, this is happening to me to a lot. Curious though I cannot reproduce this issue locally, only when running on CI environment through TeamCity build pipeline

@omnisip
Copy link
Contributor

omnisip commented Jun 30, 2020

Hey, sure here is a test case that replicates the error. I hope that helps. Please let me know if anything in my code does not make sense and I can test if changes there fix it.

Are you able to print a stack trace of where it is throwing?

@lukasoppermann
Copy link
Author

Hey @omnisip what I get is the following:

Testing Page: privacy › Testing viewport: desktop-extra-large

Error running image diff.

  67 |     })
  68 |     // compare screenshot
> 69 |     expect(image).toMatchImageSnapshot(config.setConfig({
     |                   ^
  70 |       filename: `${viewport}`,
  71 |       snapshotPath: `${config.basePath}/baseline/${currentCase.folder}`,
  72 |       diffPath: `${config.testSnaps}/${currentCase.folder}`

  at runDiffImageToSnapshot (node_modules/jest-image-snapshot/src/diff-snapshot.js:277:11)
  at Object.toMatchImageSnapshot (node_modules/jest-image-snapshot/src/index.js:194:7)
  at tests/integration/ui.baseTest.js:69:19

Does that help?

@omnisip
Copy link
Contributor

omnisip commented Jul 1, 2020 via email

@lukasoppermann
Copy link
Author

Hey @omnisip, running with runInProcess: true takes a loooot longer but does not actually produce this error (and thus no stack trace).

Does that make sense?

@omnisip
Copy link
Contributor

omnisip commented Jul 3, 2020 via email

@omnisip
Copy link
Contributor

omnisip commented Jul 12, 2020

Hey @omnisip, running with runInProcess: true takes a loooot longer but does not actually produce this error (and thus no stack trace).

Does that make sense?

Yes. I wonder if you're running out of memory in the sub process running jest-image-snapshot.

Try rerunning with the original settings but cut the jest concurrency to half or a quarter of what it currently is.

@lukasoppermann
Copy link
Author

Hey @omnisip could you please tell me how I can reduce concurrency in jest?

@omnisip
Copy link
Contributor

omnisip commented Jul 16, 2020 via email

@lukasoppermann
Copy link
Author

lukasoppermann commented Jul 20, 2020 •
edited

Sadly even with --maxWorkers=10% I still get the error for some tests. (I test with 50% first and had the same issue.)

@omnisip
Copy link
Contributor

omnisip commented Jul 20, 2020 via email

@omnisip
Copy link
Contributor

omnisip commented Jul 20, 2020

We're going to fix this if we can. I'll submit a patch today if we can get to the bottom of it.

@lukasoppermann
Copy link
Author

So even with --maxConcurrency=1 I get the issue.

@omnisip
Copy link
Contributor

omnisip commented Jul 20, 2020

What time is it there? Would you have time for a screenshare in 45 minutes to an hour?

@lukasoppermann
Copy link
Author

Sadly not, sorry. It's getting late and I have to get up really early. Is there anything more I can provide as info?

  • I am running on a macbook pro 16", 16GB on Mojave.
  • latest jest & puppeteer version
  • npm@6.14.6

@omnisip
Copy link
Contributor

omnisip commented Jul 20, 2020 via email

@lukasoppermann
Copy link
Author

What does "as a patch" mean? Do you mean a "PR" to this repo with my test case?

The original is here: https://github.com/lukasoppermann/veare/tree/master/tests/integration

@omnisip
Copy link
Contributor

omnisip commented Jul 20, 2020 via email

@lukasoppermann lukasoppermann mentioned this issue Jul 23, 2020
Closed
@lukasoppermann
Copy link
Author

Hey, sorry it took a moment, but now I got a case showing the error: #227

@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020 •
edited

Hey Lukas (@lukasopperman),

Np.  Thanks for submitting this. Before I do the deep dive, can you answer these questions?

-- Quick Questions/Comments --

  1. Is Puppeteer version 5 a hard requirement?
  2. Do you usually test against live sites?

I'm curious because Puppeteer 5 might become a breaking change for the library, and we can't keep live sites as part of the standard test suite. However... using jest-image-snapshot to do integration testing of live sites is a really cool use case.

Dan

@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020

This is failing before it finishes writing back to the parent process. So it's a genuine bug.

Do you mind if we retain a copy of your large image for the purposes of fixing it and adding it as a test case?

@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020

The particular issue is that the output size of the stringified test failure from diff-process is 16205416 bytes which is much larger than the 10MB previously allocated. I'm going to put together a pull request to do a calculation for output size instead which should resolve this issue.

omnisip added a commit to omnisip/jest-image-snapshot that referenced this issue Jul 23, 2020
@omnisip
fix: issue americanexpress#210
e9748cf 
Issue results from max buffer size being fixed to 10MB.  Replaced with
maximum allowable memory so that it becomes the imperative of the user
if the user performs tasks that cause an out of memory error.  This
behavior is then consistent with Node.JS, if they were to allocate
an extraordinarily large Buffer exceeding the available memory in
the system.
@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020

@lukasoppermann I've created a pull request with the appropriate fix. Please test it and see how it works for you.

On a separate but important note, it's currently using snapshots I derived from your test case. If this is not okay, please let us know, so we can find a suitable replacement image.

@lukasoppermann
Copy link
Author

Hey @omnisip,

to answer your questions:

  1. Puppeteer version 5 is NOT a hard requirement
  2. Do you usually test against live sites? No, but against a local version of the page.
  3. Are you asking about the image generated by the test? This is than a screenshot of theverge, which I have no legal rights to (am not working for, I just used it as a test case). I guess to be on the safe side, you should use a different page, or something.

I tested the fix by copying your changes to diff-snapshot.js into the file in the repo where I found the issue and this seems to have solved it. 👍

Thank you very much for the help & support!

@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020 via email

omnisip added a commit to omnisip/jest-image-snapshot that referenced this issue Jul 23, 2020
@omnisip
fix: issue americanexpress#210
0ce85b2 
Issue results from max buffer size being fixed to 10MB.  Replaced with
maximum allowable memory so that it becomes the imperative of the user
if the user performs tasks that cause an out of memory error.  This
behavior is then consistent with Node.JS, if they were to allocate
an extraordinarily large Buffer exceeding the available memory in
the system.
@jhildenbiddle
Copy link

@omnisip --

Not trying to hijack the thread, but I was working through memory-related issues that appear to be caused by jest-image-snapshot and discovered that setting runInProcess to true (per your advise to @lukasoppermann above) solved the problem. I've filed a separate PR here (#231), but I thought I'd mention it in case the two issues were related.

@jhildenbiddle jhildenbiddle mentioned this issue Jul 23, 2020
Closed
@omnisip
Copy link
Contributor

omnisip commented Jul 23, 2020 via email

@jhildenbiddle jhildenbiddle mentioned this issue Jul 24, 2020
Closed
@jhildenbiddle
Copy link

@omnisip -- Tested your branch. At first it seemed like the changes helped, but after a few more runs I'm starting to think there's just general flakiness with macOS CI on GitHub.

Apologies again for jumping into the thread. I was hoping for two birds with one stone, not divert attention away from @lukasoppermann's original issue.

Thx!

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity.

@bboydflo
Copy link

I recently came accross this project: https://github.com/dmtrKovalenko/odiff which could help with image diff. from what I understand odiff is much muster than other similar libraries, including the one used in this project.

It would be nice to expose an option to experimentally try odiff. What do you think?

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity.

@nellyk nellyk added bug and removed stale-issue labels Jan 18, 2021
@florianbepunkt
Copy link

@nellyk Are there any plans to address this issue? We run into the same issue when screenshooting large pages in full. A solution has been proposed by @omnisip – as alternative it would be nice to be able to make maxBuffer in runDiffImageToSnapshot configurable (while providing a safe value as default).

@tbouffard tbouffard mentioned this issue Apr 20, 2021
Open
@connorjclark connorjclark mentioned this issue Jan 15, 2022
Merged
@lukasoppermann
Copy link
Author

Stale

@lukasoppermann lukasoppermann closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2022
@Silverium
Copy link

same happened to me

@mskec
Copy link

mskec commented Aug 30, 2023 •
edited

I'm encountering the same problem when trying to compare large PNGs.
The error is:

Error running image diff: spawnSync /Users/username/.nvm/versions/node/v16.20.0/bin/node ENOBUFS
at runDiffImageToSnapshot (node_modules/jest-image-snapshot/src/diff-snapshot.js:375:11)
at Object.toMatchImageSnapshot (node_modules/jest-image-snapshot/src/index.js:219:7)

It works if I increase maxBuffers to a larger value.

jest-image-snapshot/src/diff-snapshot.js

Line 365 in 20a6e34

maxBuffer: 10 * 1024 * 1024, // 10 MB

Is this something that can be exposed through configuration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@omnisip @jhildenbiddle @lukasoppermann @bboydflo @nellyk @mskec @anescobar1991 @Kradirhamik @florianbepunkt @Silverium