Increase the maxBuffer to 10MB for the diff process #167
Conversation
|
Just out of curiosity how large are the images you are diffing that this is a problem? |
|
In my folder right now, largest snapshot is 306 KB (3072 × 1536) and largest diff is 716 KB. I’m assuming the base64 encoding pushes it over the 1 MB line.
… On Jan 22, 2020, at 7:11 PM, Andres Escobar ***@***.***> wrote:
Just out of curiosity how large are the images you are diffing that this is a problem?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#167?email_source=notifications&email_token=ABCJTBXXSPAWQEXPK7FA4LLQ7EDG3A5CNFSM4KKQA5W2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJV3UCY#issuecomment-577485323>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCJTBQQIKNACZBCQUH5DSDQ7EDG3ANCNFSM4KKQA5WQ>.
|
|
Weird… that integration test without the accompanying fix fails on my machine (macOS), but passes on Travis. OS difference maybe? I'll investigate more when I have a bit more time over the weekend. |
Gaelan
force-pushed
the
max-buffer
branch
2 times, most recently
from
3f2564c
to
93d6717
Compare
|
Alright, I added a test. Weirdly, the test (without the patch) fails both on my laptop (macOS) and in a Linux docker container, but not on Travis. Any idea why that might be the case? |
|
Maybe it is a different version of node on the travis build vs locally on your machine? |
|
Yep, that's it. Passes on Node 10, fails on Node 13. Would you be OK with adding Node 13 to the travis config? |
|
Sure let's do that, so then this bug is not an issue yet right? Only an issue on non LTS node 13? |
By default, Node's spawnSync has a maxBuffer of 1 MB. This bumps it up to 10 MB, which should reduce the chance of diffs on large images failing.
|
Now that you guys added node 12 to the travis.yml, it happens as expected: the new test fails until you add the fix, then it passes. (Note that GitHub shows the commits backwards because I rebased them to make the test come before the fix, but the timestamp still shows the test as later.) |
|
Thanks @Gaelan I see in the node docs that the default max buffer was 200 X 1024 bytes in node 10 and 1024 X 1024 bytes in node 12 so that is why there was a discrepancy. https://nodejs.org/docs/latest-v12.x/api/child_process.html#child_process_child_process_spawnsync_command_args_options |
|
@anescobar1991 anything blocking a merge on this? Appreciate your consideration on this thus far! |
# [3.0.0](v2.12.0...v3.0.0) (2020-03-24) ### Bug Fixes * **diff:** small default maxBuffer ([df713f6](df713f6)) * **diff-snapshot:** dumpDiffToConsole base64 string output ([#183](#183)) ([f73079f](f73079f)) ### chore * **packages:** updating jest to 25.1 for perf improvements ([#170](#170)) ([eb3dfa6](eb3dfa6)) * **packages:** upgrade from pixelmatch 4.x to 5.x, and pngjs to 3.4 ([#186](#186)) ([1edc9a3](1edc9a3)) * **travis:** remove node 6 from travis config ([ce2b757](ce2b757)) ### Features * **diff:** increase the maxBuffer to 10MB for the diff process ([#167](#167)) ([0927826](0927826)) ### BREAKING CHANGES * **packages:** pixelmatch is being major version bumped and so image diffs may be difference * **packages:** Node min version is now 8 * **travis:** drop support for node 6
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…icanexpress#167) By default, Node's spawnSync has a maxBuffer of 1 MB. This bumps it up to 10 MB, which should reduce the chance of diffs on large images failing.
# [3.0.0](americanexpress/jest-image-snapshot@v2.12.0...v3.0.0) (2020-03-24) ### Bug Fixes * **diff:** small default maxBuffer ([df713f6](americanexpress@df713f6)) * **diff-snapshot:** dumpDiffToConsole base64 string output ([americanexpress#183](americanexpress#183)) ([f73079f](americanexpress@f73079f)) ### chore * **packages:** updating jest to 25.1 for perf improvements ([americanexpress#170](americanexpress#170)) ([eb3dfa6](americanexpress@eb3dfa6)) * **packages:** upgrade from pixelmatch 4.x to 5.x, and pngjs to 3.4 ([americanexpress#186](americanexpress#186)) ([1edc9a3](americanexpress@1edc9a3)) * **travis:** remove node 6 from travis config ([ce2b757](americanexpress@ce2b757)) ### Features * **diff:** increase the maxBuffer to 10MB for the diff process ([americanexpress#167](americanexpress#167)) ([0927826](americanexpress@0927826)) ### BREAKING CHANGES * **packages:** pixelmatch is being major version bumped and so image diffs may be difference * **packages:** Node min version is now 8 * **travis:** drop support for node 6
By default, Node's spawnSync has a maxBuffer of 1 MB. This bumps it up to 10 MB, which should reduce the chance of diffs on large images failing.