-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Binary stream corruption fix #352
base: master
Are you sure you want to change the base?
Conversation
Seems related to #351 |
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.
Needs regression tests still
I think that the test should be the same as this test by just removing "encoding: false". |
This fix seems "obviously" correct -- somehow this typo sneaked through the whole vetting or there was some merging weirdness. But I don't yet understand why this branch of the code was never reached by the tests. It's been a long time since I looked at this and there's some things I don't recognize (such as a suite of stream backends, is that right?) |
There are currently no e2e binary stream tests. Here is the example of failing test, please add to the most suitable test file. it('should not destroy binary file', function (done) {
var ranBomInputPath = path.join(testConstants.inputBase, './ranbom.bin');
var ranBomOutputPath = path.join(testConstants.outputBase, './ranbom.bin');
function assert(files) {
var srcResult = fs.readFileSync(ranBomInputPath, null);
var destResult = fs.readFileSync(ranBomOutputPath, null);
expect(srcResult).toEqual(destResult);
}
pipeline(
[
vfs.src(ranBomInputPath, { encoding: false, buffer: false }),
vfs.dest(testConstants.outputBase),
concatArray(assert),
],
done
);
}); |
Gentle ping, any progress on this? |
Addresses gulpjs/gulp#2803