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

feat: inline images support (dumpDiffToConsole='inline') #244

Merged
merged 2 commits into from
Dec 16, 2020

Conversation

LukeChannings
Copy link
Contributor

This MR implements support for iTerm's Inline Images Protocol when the dumpDiffToConsole flag is set to 'inline'.

The scope of this is admittedly narrow, but it may be useful to a wider audience.

@LukeChannings LukeChannings requested a review from a team as a code owner November 7, 2020 16:24
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2020

CLA assistant check
All committers have signed the CLA.

README.md Outdated
@@ -117,7 +117,9 @@ See [the examples](./examples/README.md) for more detailed usage or read about a
* `updatePassedSnapshot`: (default `false`) Updates a snapshot even if it passed the threshold against the existing one.
* `blur`: (default `0`) Applies Gaussian Blur on compared images, accepts radius in pixels as value. Useful when you have noise after scaling images per different resolutions on your target website, usually setting its value to 1-2 should be enough to solve that problem.
* `runInProcess`: (default `false`) Runs the diff in process without spawning a child process.
* `dumpDiffToConsole`: (default `false`) Will output base64 string of a diff image to console in case of failed tests (in addition to creating a diff image). This string can be copy-pasted to a browser address string to preview the diff for a failed test.
* `dumpDiffToConsole`: (default `false`)
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to have the options be true, false,inline doesn't it? Since there is both a boolean and a string mixed in.

Can we instead detect if running in iTerm and if so display the image inline if dumpDiffToConsole is true?

Or does this bloat the output too much? Can you share screenshots of what it would look like?

Copy link
Contributor Author

@LukeChannings LukeChannings Nov 10, 2020

Choose a reason for hiding this comment

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

Hey Andres,

Here's what it looks like in iTerm,

Screenshot 2020-11-10 at 09 26 08

In Terminal.app it looks like this:

Screenshot 2020-11-10 at 09 42 31

With regards to mixing boolean and inline, I mostly just wanted to have an explicit option that re-used the exiting glue code. As for the relative oddness of the type itself, I guess that's up to you to decide, but the type is representable in the TypeScript definitions without any problems.

Detection

It might be possible to detect support for the Inline Images Protocol, but I'd prefer to do it based on the feature itself rather than the just checking if the Terminal is iTerm. This is so that the feature still works for other Terminals that support it (the options are slim at the moment).

This might add a lot of code that's full of OSC escape sequences and would likely be a bit of a maintenance burden. It might also not be possible to do reliably...

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about a new option, enableInlineConsoleDiff or something to that effect?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah I think either a new option like @igetgames is suggesting (there are so many now!) might work!

@americanexpress/one what do you all think? New flag or mix booleans with string in the existing option?

Copy link
Contributor Author

@LukeChannings LukeChannings Nov 18, 2020

Choose a reason for hiding this comment

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

Hey folks,

I've updated the MR to add a new option: dumpInlineDiffToConsole, the existing dumpDiffToConsole is left as-is.
In addition if the terminal isn't supported (or ENABLE_INLINE_DIFF is in the env), we fallback to dumpDiffToConsole.

@anescobar1991 @igetgames

anescobar1991
anescobar1991 previously approved these changes Dec 11, 2020
@LukeChannings
Copy link
Contributor Author

@anescobar1991 I've just simplified the code a little bit and fixed an issue with iTerm. Do you mind ticking again?

anescobar1991
anescobar1991 previously approved these changes Dec 11, 2020
@anescobar1991
Copy link
Member

@igetgames do you mind reviewing so we can get this in hopefully today?

@LukeChannings
Copy link
Contributor Author

image

@anescobar1991 Ok this one should actually work 😅

anescobar1991
anescobar1991 previously approved these changes Dec 11, 2020
marcusrbrown
marcusrbrown previously approved these changes Dec 11, 2020
Copy link
Contributor

@marcusrbrown marcusrbrown left a comment

Choose a reason for hiding this comment

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

Great feature, thank you!

@marcusrbrown
Copy link
Contributor

@LukeChannings can you squash your commits?

@LukeChannings
Copy link
Contributor Author

@igetgames Done!

@LukeChannings
Copy link
Contributor Author

@igetgames Can you tick please?

@marcusrbrown marcusrbrown merged commit b82b068 into americanexpress:master Dec 16, 2020
oneamexbot added a commit that referenced this pull request Dec 16, 2020
# [4.3.0](v4.2.0...v4.3.0) (2020-12-16)

### Features

* inline images support ([#244](#244)) ([b82b068](b82b068))
@oneamexbot
Copy link
Contributor

🎉 This PR is included in version 4.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023
goverdhan07 pushed a commit to goverdhan07/jest-image-snapshot that referenced this pull request Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants