jest-image-snapshot icon indicating copy to clipboard operation
jest-image-snapshot copied to clipboard

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

Open lukasoppermann opened this issue 5 years ago • 34 comments

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)?

lukasoppermann avatar May 19 '20 12:05 lukasoppermann

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.

anescobar1991 avatar May 26 '20 15:05 anescobar1991

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()
  })

lukasoppermann avatar May 28 '20 04:05 lukasoppermann

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

Kradirhamik avatar Jun 12 '20 12:06 Kradirhamik

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?

omnisip avatar Jun 30 '20 17:06 omnisip

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?

lukasoppermann avatar Jul 01 '20 06:07 lukasoppermann

Yes. Very helpful.

There's an option to run jest image snapshot in process.

Can you add runInProcess: true as soon of the options you pass toMatchSnapshot, and paste the next stack trace here as well?

On Wed, Jul 1, 2020, 00:33 Lukas Oppermann [email protected] wrote:

Hey @omnisip https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-652221204, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJXNA3SNDRIRW3TDAA3RZLKC5ANCNFSM4NE6LOWQ .

omnisip avatar Jul 01 '20 20:07 omnisip

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?

lukasoppermann avatar Jul 03 '20 08:07 lukasoppermann

Hmm...

On Fri, Jul 3, 2020, 02:09 Lukas Oppermann [email protected] wrote:

Hey @omnisip https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-653414792, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJSTVGF7NHMEQH7ILM3RZWG5BANCNFSM4NE6LOWQ .

omnisip avatar Jul 03 '20 14:07 omnisip

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.

omnisip avatar Jul 12 '20 00:07 omnisip

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

lukasoppermann avatar Jul 12 '20 12:07 lukasoppermann

--maxWorkers=50%

On Sun, Jul 12, 2020, 06:01 Lukas Oppermann [email protected] wrote:

Hey @omnisip https://github.com/omnisip could you please tell me how I can reduce concurrency in jest?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-657212373, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJWVWB2LDWDQAE3IRGTR3GQYNANCNFSM4NE6LOWQ .

omnisip avatar Jul 16 '20 20:07 omnisip

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

lukasoppermann avatar Jul 20 '20 12:07 lukasoppermann

And just for grins, can you set --maxConcurrency=1 as well?

On Mon, Jul 20, 2020, 06:01 Lukas Oppermann [email protected] wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-660983994, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJSOXZEMN23X3FKFOQLR4QWYBANCNFSM4NE6LOWQ .

omnisip avatar Jul 20 '20 19:07 omnisip

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

omnisip avatar Jul 20 '20 19:07 omnisip

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

lukasoppermann avatar Jul 20 '20 19:07 lukasoppermann

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

omnisip avatar Jul 20 '20 19:07 omnisip

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
  • [email protected]

lukasoppermann avatar Jul 20 '20 19:07 lukasoppermann

Yeah. You provided a test case, but it's not integrated into the jest image snapshot test suite.

Are you able to provide that test as a patch so we can play with it?

On Mon, Jul 20, 2020, 13:25 Lukas Oppermann [email protected] wrote:

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
  • [email protected]

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-661286687, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJTC7WNNF7SOPNKPNX3R4SKZFANCNFSM4NE6LOWQ .

omnisip avatar Jul 20 '20 19:07 omnisip

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

lukasoppermann avatar Jul 20 '20 19:07 lukasoppermann

Yes. A PR would be perfect.

On Mon, Jul 20, 2020, 13:41 Lukas Oppermann [email protected] wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-661294035, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJT7QTILPJR6HIJAZYDR4SMVNANCNFSM4NE6LOWQ .

omnisip avatar Jul 20 '20 20:07 omnisip

Hey, sorry it took a moment, but now I got a case showing the error: https://github.com/americanexpress/jest-image-snapshot/pull/227

lukasoppermann avatar Jul 23 '20 10:07 lukasoppermann

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 avatar Jul 23 '20 16:07 omnisip

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 avatar Jul 23 '20 17:07 omnisip

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 avatar Jul 23 '20 17:07 omnisip

@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.

omnisip avatar Jul 23 '20 19:07 omnisip

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!

lukasoppermann avatar Jul 23 '20 20:07 lukasoppermann

You're welcome. Let's see what we can do about finding a really large PNG file...

On Thu, Jul 23, 2020 at 8:06 PM Lukas Oppermann [email protected] wrote:

Hey @omnisip https://github.com/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!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-663206481, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJWQVFJT47Y4MIGZOSDR5CJ3XANCNFSM4NE6LOWQ .

omnisip avatar Jul 23 '20 20:07 omnisip

@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 avatar Jul 23 '20 21:07 jhildenbiddle

Feel free to try this pull request branch without runInProcess and let me know.

On Thu, Jul 23, 2020 at 9:45 PM John Hildenbiddle [email protected] wrote:

@omnisip https://github.com/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 https://github.com/lukasoppermann above) solved the problem. I've filed a separate PR here (#231 https://github.com/americanexpress/jest-image-snapshot/issues/231), but I thought I'd mention it in case the two issues were related.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/americanexpress/jest-image-snapshot/issues/210#issuecomment-663247795, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQJUF2VAD65EJUBPKUODR5CVOPANCNFSM4NE6LOWQ .

omnisip avatar Jul 23 '20 22:07 omnisip

@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!

jhildenbiddle avatar Jul 24 '20 06:07 jhildenbiddle