Skip to content
This repository has been archived by the owner on Mar 21, 2021. It is now read-only.

feature request: screenshot diffing #15

Closed
ariesshrimp opened this issue Jul 12, 2017 · 34 comments
Closed

feature request: screenshot diffing #15

ariesshrimp opened this issue Jul 12, 2017 · 34 comments

Comments

@ariesshrimp
Copy link

Here is my dream api:

// in jest
describe('app', () =>
  it('should not change', async () => {
    const myComponentLib = 'localhost:6006'
    const filepath = __dirname
    // an image?
    const actual = await chrome.goto(myComponentLib)
      .then(() => chrome.screenshot(filepath))
      .then(() => chrome.done());
    const expected = await theMostRecentImageOnDisk
    expect(actual).toEqual(expected)
    // if this throws, prompt me with a comparison of the two images side by side
    // let me approve the overwrite or accept the failure
    // ala jest snapshots
  })
)

i guess the screenshot could be enough if i had a good diffing library, but the hard part is still prompting users with the comparison side-by-side, and then continuing the tests after input is received.

this feels like an overlap of concerns between the test runner, the assertion lib, and the driver, but nobody is doing this well right now. jest html snapshots have the perfect workflow, it's just that they're nonsense text output instead of pictures.

@joelgriffith
Copy link
Owner

I like this idea a lot, and there's already an excellent package for doing image based snapshots.

There's a few tweaks that need to happen in Navalia to make this happen, but other than that it's just an education problem. I also need to get a RECIPES type file in this project, and this is a great opportunity for that

@ariesshrimp
Copy link
Author

ariesshrimp commented Jul 12, 2017

Cool, I'm happy to discuss a plan and contribute to that effort. It would definitely pay off for me! 🔥

What package do you have in mind for the snapshotting?

@joelgriffith
Copy link
Owner

Awesome! Here's my opinion thus far (I'm open to feedback):

  • Expose the ability to not pass in a file-path in the screenshot method (should be located here: src/Chrome.ts. Not passing in a FP will instead return the base64 string that Chrome generates for the screenshot.

  • Recommend something like: https://github.com/americanexpress/jest-image-snapshot for screenshot diffing in Jest. We can easily add a HOWTO piece of documentation for how to achieve this type of workflow.

  • Profit

Since Jest has a nice API for diffing deltas (snapshoting) I'd rather not build the diff-ing portion in this repo. jest-image-snapshot might fit the bill, but I haven't investigated what they're using internally to do the delta's (whereas something like https://github.com/mapbox/pixelmatch is doing it via array-buffers === faster and less setup confusion).

I think for this to really do well we'll probably have to paint the diff inside a terminal window... which might be tricky to achieve...

@ariesshrimp
Copy link
Author

ariesshrimp commented Jul 12, 2017

I agree, it seems awkward to include the diff engine here.

So I'm hearing that:

  1. we can get the base64 string from a screenshot 👍
  • actionable item: expose this api
  1. We can use that to compare against other base64 strings and just leave that up to pixelmatch or jest 👍
  • actionable item: document this integration
  1. We can make assertions about the differences 👍
  • actionable item: document this strategy.

all low-hanging fruit.

but the thing I'm still not seeing is the developer niceties, like being presented with the differences, and not needing to manually manage the canonical base64 string. those are things that i want to be the purview of the tools. do you think those are features in the scope of this project? Part of what's so convenient about the jest snapshot workflow is that you don't have to keep track of your canonical image files by hand.

@joelgriffith
Copy link
Owner

but the thing I'm still not seeing is the developer niceties, like being presented with the differences, and not needing to manually manage the canonical base64 string

On point. We'd need some way to present to the user the delta of the images, and for now I think this means rendering the delta inside a terminal (which is somewhat tricky). This I don't have a good answer for, but I'd be shocked if it didn't already exist.

Another approach would be to set a threshold of changes, and when it's breached prompt the user that a significant change has taken place and expose a link to the delta? That way we're only bound to showing a message as opposed to fully rendering inside a terminal.

What do you think your ideal workflow would be?

@ariesshrimp
Copy link
Author

personally i think a link would be even better than the terminal approach

@joelgriffith
Copy link
Owner

All the work in Navalia is there now to support this in 0.0.17. screenshot now returns a base-64 encoded buffer when no filepath is supplied. This allows for custom Jest methods to consume that, save snapshots, and do diffing.

I can start work on this Jest extension, but it's up for grabs as well :) I was going to pursue using https://github.com/mapbox/pixelmatch since it operates on Buffer'd data, and requires no outside libs like imagemagick

@ariesshrimp
Copy link
Author

Cool. I guess the order might be:

  1. Prove that pixelmatch diffs correctly and that a good api interface is possible.
  2. Demonstrate linking Jest as a consumer.
  • I guess this is the part that would store images for comparison, handle clean-up, etc
  1. Handle the diffing display. Which might involve demoing several different outputs/feedback loops. For example, I'm thinking about test watchers. I'm not sure how much of this Jest makes easy or difficult.
  2. Publicly document the results.

I think (1) and (2) can happen independently. Have you already used pixelmatch or written a Jest interface in the past? I haven't done either, but could try both.

@joelgriffith
Copy link
Owner

Yeah, 1 and 2 I haven't attempted. pixelmatch looks fairly easy to consume, however the bulk of work is in writing the expect extension for Jest as it will have to:

  • Deal with the snapshotting (the snapshot extension API only has hooks for printing, not low-level file-handling), and persisting of snapshots
  • Constructing failures + messaging
  • Draw the diff, set it into a temporary place, and exposing it in the CLI

I think this would be a good MVP for the desired features, and we can iterate on it to make any improvements. For reference, here's a library that does effectively that https://www.npmjs.com/package/jest-image-snapshot. Might even consider using that to see how it does?

@anescobar1991
Copy link

Hey guys I'm the maintainer for jest-image-snapshot and I'd be happy to collaborate with you guys on this! I haven't tried it yet but the way I see it you should be able to get a screenshot from chrome using your API and then just pass it to toMatchImageSnapshot().

toMatchImageSnapshot() already replicates all of the jest toMatchSnapshot() functionality, handles the failure messages and the places the diff in the file directory while exposing it on the CLI.

If there is something missing that you guys need let me know though as I am looking into using navaliafor my team's use cases and would love to improve on it.

@joelgriffith
Copy link
Owner

@anescobar1991 nice!! I think that would fit the features in this issue, though I haven't tried it out yet. The biggest "gotta-have" was the ability to present some sort of diff to the user (which a link in to the png/image in the file-system is great). I'll give this a whirl and report back here if there's an issue.

@ariesshrimp
Copy link
Author

ariesshrimp commented Jul 14, 2017

@anescobar1991 sounds great! we were talking over in americanexpress/jest-image-snapshot#1

the dream api here would be if toMatchImageSnapshot() accepted a base64 encoded buffer directly.

    // actual is a base64 buffer
    const actual = await chrome.goto(myComponentLib).then(() => chrome.screenshot())
    expect.toMatchImageSnapshot(actual) 🔥

then i don't have to bother saving or reading anything by hand

@joelgriffith
Copy link
Owner

We can (probably easily) translate the base64 buffer into just a good-ol' buffer to get the API's aligned

@anescobar1991
Copy link

awesome! Let me know how it works. Without actually trying it out I think you can without any modification to either library do something like:

  const actual = await chrome
      .goTo(myComponentLib)
      .screenshot();
  expect(actual).toMatchImageSnapshot();

The reason I am saying it should work is that all I am doing is passing the image data directly to blink-diff which is what I use to do the actual image comparison and according to the blink-diff docs either a "PNGImage instance or a Buffer instance with PNG data" should work.

@joelgriffith
Copy link
Owner

@joefraley have you attempted the above? My suspicion is that the buffer we have isn't encoded properly for consumption by PNGImage

@ariesshrimp
Copy link
Author

yeah it didn't seem to work last i checked. blink-diff was throwing internally. let me repro real quick

@anescobar1991
Copy link

anescobar1991 commented Jul 18, 2017

Oddly enough it kinda worked for me, no thrown errors and a written snapshot. Snapshot I got was just a blank png though but I believe that is a chrome headless issue as I was seeing that before even with Selenium when running headless.

Are you guys seeing the same thing? I'm running Mac OS 10.12.5 with Chrome 59.0.3071.115 and navalia@0.0.22

@joelgriffith
Copy link
Owner

I've noticed (and seen) issues in Chrome 59.x.x.x. I have Canary installed which generally seems to function properly with screenshots (Navalia will default to Canary over vanilla Chrome when present).

I'll try and give this some hands-on time tomorrow

@anescobar1991
Copy link

anescobar1991 commented Jul 18, 2017

Got it working with no modification! I discovered your undocumented way to pass chrome some flags and ran it without headless and got an image snapshot!

const chrome = new Chrome({ flags: { headless: false } });

it('should work', () => {
  await chrome.goto(url);
  await chrome.wait(elementToWaitOn);
  const image = await chrome.screenshot();
  await chrome.done();
  expect(image).toMatchImageSnapshot();
});

@joelgriffith
Copy link
Owner

Holy smokes! Nice work!! I thought I had it documented... (probably just buried) https://joelgriffith.github.io/navalia/chrome/constructor/. Probably should document it in the code as well.

This is a really nice feature that I'd like to write something about. Not sure if you'd be interested in sharing (Medium or whatever)? Could co-author the article? PEOPLE NEED TO KNOW!

@joelgriffith
Copy link
Owner

Might also be good to get a discord channel or something like that going for faster iteration on stuff like this

@anescobar1991
Copy link

Yeah I'd love to co author an article about this, just let me know where and how we can collaborate on it.

@ariesshrimp
Copy link
Author

ariesshrimp commented Jul 18, 2017

wow, good job @anescobar1991 🎉! @joelgriffith where does that leave the status of this request? personally this discovery meets all of my immediate needs

@joelgriffith
Copy link
Owner

I'm going to close this out @joefraley, but we can continue discussing how to get this out to a wider audience. I think I'll try and write something up on Medium, have you and @anescobar1991 make any edits/tweaks you'd like?

@joelgriffith
Copy link
Owner

@joefraley and @anescobar1991 I've done a pretty fast write-up here: https://medium.com/@griffith_joel/automatic-visual-regression-testing-23cc06471dd, leave whatever comments you'd like and I can publish it (also can tweet/share/mail/friendster/myspace it around)

@ariesshrimp
Copy link
Author

Cool, I'll get a screenshot/gif for the article this evening!

@anescobar1991
Copy link

anescobar1991 commented Jul 19, 2017

Awesome I will take a look later today! And definitely need to myspace it. Very important medium to communicate this.

@joelgriffith
Copy link
Owner

Thanks for the feedback! I've updated and did some tweaks to the article
@anescobar1991 could you give it a quick read through? Some sections have changed a bit to accommodate your feedback
@joefraley if you don't have time I can do the gif preview.

@anescobar1991
Copy link

Looks great! Ship it!

@joelgriffith
Copy link
Owner

@anescobar1991
Copy link

Llooks really great! Any plan to get it on reddit, hackernews, etc...?

@joelgriffith
Copy link
Owner

I've made some attempts.... HN doesn't really give me much love. If you know someone with some "push" there let me know (same with Reddit). I'll upvote away!

@anescobar1991
Copy link

Yeah I'll see if I can find someone. If it's any help the reason I found Navalia and subsequently this issue was from reddit. So it does work whether it looks like you're gaining traction or not.

@joelgriffith
Copy link
Owner

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants