Skip to content

Commit

Permalink
fix: issue americanexpress#210
Browse files Browse the repository at this point in the history
Issue results from max buffer size being fixed to 10MB.  Replaced with
maximum allowable memory so that it becomes the imperative of the user
if the user performs tasks that cause an out of memory error.  This
behavior is then consistent with Node.JS, if they were to allocate
an extraordinarily large Buffer exceeding the available memory in
the system.
  • Loading branch information
omnisip committed Jul 23, 2020
1 parent 640fe90 commit e9748cf
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 1 deletion.
29 changes: 29 additions & 0 deletions __tests__/integration.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2017 American Express Travel Related Services Company, Inc.
* Copyright (c) 2020 Dan Weber <dweber@gmail.com>
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand Down Expand Up @@ -425,6 +426,34 @@ describe('toMatchImageSnapshot', () => {
).toThrow(/Expected image to match or be a close match/);
});

it('diff-process should succeed even if the tests fail with large image sizes (issue #210)', () => {
const superLargeImageData = fs.readFileSync(fromStubs('verge-1400-snap.png'));
const superLargeImageFailureData = fs.readFileSync(fromStubs('verge-1400-snap-bad.png'));
const customSnapshotIdentifier = getIdentifierIndicatingCleanupIsRequired();
// First we need to write a new snapshot image
expect(
() => expect(superLargeImageData)
.toMatchImageSnapshot({
failureThreshold: 0.00,
failureThresholdType: 'pixel',
customSnapshotIdentifier,
})
)
.not
.toThrowError();

// then test against a different image
expect(
() => expect(superLargeImageFailureData)
.toMatchImageSnapshot({
failureThreshold: 0,
failureThresholdType: 'pixel',
customSnapshotIdentifier,
})
)
.toThrowError(/Expected image to match or be a close match/);
});

describe('Desktop Images Test', () => {
it('not to throw at 6pct with pixelmatch with', () => {
const largeImageData = fs.readFileSync(fromStubs('Desktop 1_082.png'));
Expand Down
Binary file added __tests__/stubs/verge-1400-snap-bad.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added __tests__/stubs/verge-1400-snap.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 17 additions & 1 deletion src/diff-snapshot.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2017 American Express Travel Related Services Company, Inc.
* Copyright (c) 2020 Dan Weber <dweber@gmail.com>
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand Down Expand Up @@ -335,7 +336,22 @@ function runDiffImageToSnapshot(options) {
{
input: Buffer.from(serializedInput),
stdio: ['pipe', 'inherit', 'inherit', 'pipe'],
maxBuffer: 10 * 1024 * 1024, // 10 MB
// Dan (@omnisip): Before setting this, I checked the implementation
// of Node.JS's child_process all the way back to v0.10.48.
// This value does not cause Node.js to allocate memory. On the contrary,
// it exists only to serve as a validation for the maximum data to be returned.
// If the output amount is exceeded, node will automatically terminate the process.
// As such, we have a couple options:
// 1) We can guess as to how much data it will actually need and throttle it
// (e.g. 4*height*width*3 + expected JSON size); or
// 2) We can allow it be as much memory as required (in this case, Number.MAX_SAFE_INTEGER)
// Technically neither is ideal, but since we store entire and process images in memory,
// the amount of memory used will always be the same. As such, it seems most apt
// to allow the user to deal with out of memory issues as they arise using the standard
// operating system delivery methods than for us to establish an arbitrary limit on
// what they can and can't do. If this becomes a legitimate issue in the future,
// we can always look at supporting images as streams from disk.
maxBuffer: Number.MAX_SAFE_INTEGER,
}
);

Expand Down

0 comments on commit e9748cf

Please sign in to comment.