-
Notifications
You must be signed in to change notification settings - Fork 203
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
Allow to generate diff for images with different sizes #42
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,61 @@ const pixelmatch = require('pixelmatch'); | |
const mkdirp = require('mkdirp'); | ||
const { PNG } = require('pngjs'); | ||
|
||
/** | ||
* Helper function to create reusable image resizer | ||
*/ | ||
const createImageResizer = (width, height) => (source) => { | ||
const resized = new PNG({ width, height, fill: true }); | ||
PNG.bitblt(source, resized, 0, 0, source.width, source.height, 0, 0); | ||
return resized; | ||
}; | ||
|
||
/** | ||
* Fills diff area with black transparent color for meaningful diff | ||
*/ | ||
/* eslint-disable no-plusplus, no-param-reassign, no-bitwise */ | ||
const fillSizeDifference = (width, height) => (image) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function just fills new area added after resize with transparent black color. I like idea of checker board pattern, but it seems to be too complicated to implement considering how low-level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah the worry we had was that just filling it with color would confuse the user as they would think that the color was part of their passed in image. Since you made it transparent though when I was running it locally it seemed clear enough that the black color was not a part of the image so this should be fine from my POV at least. |
||
const inArea = (x, y) => y > height || x > width; | ||
for (let y = 0; y < image.height; y++) { | ||
for (let x = 0; x < image.width; x++) { | ||
if (inArea(x, y)) { | ||
const idx = ((image.width * y) + x) << 2; | ||
image.data[idx] = 0; | ||
image.data[idx + 1] = 0; | ||
image.data[idx + 2] = 0; | ||
image.data[idx + 3] = 64; | ||
} | ||
} | ||
} | ||
return image; | ||
}; | ||
/* eslint-enabled */ | ||
|
||
/** | ||
* Aligns images sizes to biggest common value | ||
* and fills new pixels with transparent pixels | ||
*/ | ||
const alignImagesToSameSize = (firstImage, secondImage) => { | ||
// Keep original sizes to fill extended area later | ||
const firstImageWidth = firstImage.width; | ||
const firstImageHeight = firstImage.height; | ||
const secondImageWidth = secondImage.width; | ||
const secondImageHeight = secondImage.height; | ||
// Calculate biggest common values | ||
const resizeToSameSize = createImageResizer( | ||
Math.max(firstImageWidth, secondImageWidth), | ||
Math.max(firstImageHeight, secondImageHeight) | ||
); | ||
// Resize both images | ||
const resizedFirst = resizeToSameSize(firstImage); | ||
const resizedSecond = resizeToSameSize(secondImage); | ||
// Fill resized area with black transparent pixels | ||
return [ | ||
fillSizeDifference(firstImageWidth, firstImageHeight)(resizedFirst), | ||
fillSizeDifference(secondImageWidth, secondImageHeight)(resizedSecond), | ||
]; | ||
}; | ||
|
||
function diffImageToSnapshot(options) { | ||
const { | ||
receivedImageBuffer, | ||
|
@@ -45,14 +100,16 @@ function diffImageToSnapshot(options) { | |
|
||
const diffConfig = Object.assign({}, defaultDiffConfig, customDiffConfig); | ||
|
||
const receivedImage = PNG.sync.read(receivedImageBuffer); | ||
const baselineImage = PNG.sync.read(fs.readFileSync(baselineSnapshotPath)); | ||
|
||
if ( | ||
receivedImage.height !== baselineImage.height || receivedImage.width !== baselineImage.width | ||
) { | ||
throw new Error('toMatchImageSnapshot(): Received image size must match baseline snapshot size in order to make comparison.'); | ||
} | ||
const rawReceivedImage = PNG.sync.read(receivedImageBuffer); | ||
const rawBaselineImage = PNG.sync.read(fs.readFileSync(baselineSnapshotPath)); | ||
const hasSizeMismatch = ( | ||
rawReceivedImage.height !== rawBaselineImage.height || | ||
rawReceivedImage.width !== rawBaselineImage.width | ||
); | ||
// Align images in size if different | ||
const [receivedImage, baselineImage] = hasSizeMismatch | ||
? alignImagesToSameSize(rawReceivedImage, rawBaselineImage) | ||
: [rawReceivedImage, rawBaselineImage]; | ||
const imageWidth = receivedImage.width; | ||
const imageHeight = receivedImage.height; | ||
const diffImage = new PNG({ width: imageWidth, height: imageHeight }); | ||
|
@@ -70,7 +127,10 @@ function diffImageToSnapshot(options) { | |
const diffRatio = diffPixelCount / totalPixels; | ||
|
||
let pass = false; | ||
if (failureThresholdType === 'pixel') { | ||
if (hasSizeMismatch) { | ||
// Always fail test on image size mismatch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we explicitly set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, done |
||
pass = false; | ||
} else if (failureThresholdType === 'pixel') { | ||
pass = diffPixelCount <= failureThreshold; | ||
} else if (failureThresholdType === 'percent') { | ||
pass = diffRatio <= failureThreshold; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with disabling
no-plusplus
since in a loop that is a pretty standard way to iterate that has been around since the beginning of programming. Andno-bitwise
since you do need it.However you should definitely not disable
no-param-reassign
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-param-reassign
required to mute errors for reassigningimage.data
value