Auto-Replace Snapshot

[joeblau]

One feature of @orta original application was the ability to update a broken snapshot by just clicking a button.

This is useful in the event where you change text and want to update without having to disable and enable recording between tests. This might need some design, but some way to replace broken snapshots with new updates would be awesome.

[Antondomashnev]

Hi @joeblau, sounds like a cool idea actually, I will definitely add it to the roadmap 👌

[joeblau]

Do you have a designer that's helping out with the project or any design ideas around addressing this?

Here is the original design, but my colleagues and I want a faster way to preview and run though the snapshots to decide which ones we want to keep.

[Antondomashnev]

Not really, but @orta had good ideas for the design here #19, you're welcome to share your thoughts 😄

but my colleagues and I want a faster way to preview and run though the snapshots to decide which ones we want to keep.

Could you elaborate on that, please.

[joeblau]

In Snapshots, you have to click on each failed snapshot and then click "swap" to replace the broken snapshot with the fixed snapshot.

What we were thinking of is a way to either bulk accept / reject snapshots with checkboxes or have a way to go though the snapshots like tinder where you can say "keep old one" or "keep new one" quickly, I think that would be helpful.

[Antondomashnev]

I like bulk accept / reject snapshots with checkboxes option 😄

As I understood from this:

click on each failed snapshot and then click "swap" to replace the broken snapshot with the fixed snapshot.

It requires two steps to swap the snapshot and the marking the checkbox only one. So maybe there could be a button "swap" in the UI, so you don't have to click on the failed snapshot and then click "swap", but you can swap in one step. What do you think?

[orta]

I've always been partial to the photos like experience for work like this:

So that you can see the images in big, then move left/right with arrows and press spacebar to switch it. The mouse can then be used for scrubbing between the two.

( I don't have a big monitor so I'm not doing anything fine-grained)

[Antondomashnev]

@orta I like how full-size mode could look like 👍 , also I think it could be an option. From my experience sometimes it's just faster to check everything in a pop-up mode if you don't have to pay attention to details.

[orta]

Definitely, think those are two different use cases 👍

[Antondomashnev]

Hey @joeblau I've done some improvements towards your request. Here is a preview:
.

Basically the idea is that you have now sections. Section contains multiple snapshots from the same test file. Each section has swap action - that swaps all snapshots that can be swapped in that section. Also each snapshot has the same action - to swap only itself. I've been thinking also about swap button that swaps all snapshots from all sections, but it feels a bit scary for me.
I'm looking forward to hearing your feedback 🚀

[joeblau]

Oh... my... god!

Thanks so much. I'm going to test this out on wed when I get back to work.

[babbage]

Just started using this app, and it's great. I suggest "Swap" isn't quite the right term, as it implies that you are swapping the two screenshots: not just that the new "failed" screenshot is becoming the reference, but also that the reference is being moved back to the location of the failed one.

Two minor suggestions then:

1. Rename "Swap" to "Accept" and the one for the entire section to "Accept All".
2. When someone Accepts the new screenshot as a reference screenshot, I'd suggest removing all three associated images from the FailureDiffs folder, or at least have an option for this. That way, once you've accepted the intended changes, all that is left there are the ones that need attention.

Happy to take a stab at this and submit a pull request if that's preferred, but wanted to propose it before I took any action.

[orta]

Agree - think this makes sense

[Antondomashnev]

@babbage I like the idea, and the wording makes more sense than the current one. Would be happy to review your PR 😄

Feel free to ping me if you have any question regarding the implementation.

[babbage]

I have implemented the renaming in the interface and in the naming of properties and functions throughout the code in this branch:
https://github.com/babbage/FBSnapshotsViewer/tree/Accept

It doesn't yet provide the planned option to remove the related images from the FailureDiffs folder on acceptance, so I haven't created a pull request yet. However, I would value some assistance. In that branch I have corrected the issues in the current master branch flagged by the linter, but am getting a fatal exception on trying to run the tests in the current code base, and have determined that there is a fatal error in the tests in the current Master branch—it's not due to my changes.

The fatal error is seen associated with this assertionFailure:

assertionFailure("Invalid data reported by KZFileWatchers.FileWatcher.Local")

The relevant test being run is [FBSnapshotsViewerTests.ApplicationSnapshotTestResultFileWatcherUpdateHandlerSpec _handleFileWatcherUpdate__with_invalid_updates__thows_assertion], which is expected to throw an assertion. What I am unclear about is why this is failing. Possibly this is an environment configuration issue? I am not familiar enough with the rest of the codebase to be able to debug this promptly. It does seem that there may be issues with Nimble here (e.g., see related Quick/Quick#20 though that does not describe a cause that I found in the code in this case).

Is this a known issue perhaps?

[babbage]

Ah, it looks like this current issue is probably the cause:
Quick/Nimble#478

[babbage]

All fixed, and have pushed to the same branch a fix for the one failing test currently in Master, which is unrelated to these other changes but hopefully acceptable to include in the one pull request. Will proceed on with the proposed feature addition of the option to remove from FailureDiffs images associated with an accepted screenshot.

[Antondomashnev]

Hi @babbage thanks for doing a great job 😄
May I ask you to submit first the PR with the rename and then another PR with the deletion of the images. That would simplify the review a lot 😉

[babbage]

Sure.

[babbage]

Done. Apologies for the extended delay in finalising this, after the work was basically done in November. Life. :)

I am making each pull request (these and the others I've submitted) entirely independent, each off the current master, but am also maintaining a fully merged branch with all of my pull requests here. These two pull requests above in particular require a minor piece of merge work:
https://github.com/babbage/FBSnapshotsViewer/tree/Integrated_Changes

[Antondomashnev]

No worries @babbage!
Thanks for your work 😄