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

Add integration test for large file upload #1094

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
master:
chores:
- GH-1094 Added integration test for large files upload

7.26.7:
date: 2020-10-07
chores:
Expand Down
23 changes: 23 additions & 0 deletions test/fixtures/servers/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,27 @@ httpServer.on('/custom-reason', function (req, res) {
res.end();
});

httpServer.on('/upload', function (req, res) {
if (req.method === 'POST') {
Copy link
Member

Choose a reason for hiding this comment

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

Why it only supports POST?

Copy link
Author

Choose a reason for hiding this comment

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

Was supporting POST only as currently API was only being used for upload files. Have added GET

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was method doesn't matter. Don't have method-specific checks.

let body = [];

req.on('data', function (data) {
body.push(data);
Copy link
Member

Choose a reason for hiding this comment

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

Buffer has a concat method.

Copy link
Author

Choose a reason for hiding this comment

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

Rather than calling concat() each time on buffer which will be slow can't we push all the buffers in the array and then call concat() at once like Buffer.concat(body). Please let me know if there is any issue with this approach just for learning.

Copy link
Member

Choose a reason for hiding this comment

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

Have you benchmarked? Share the results.

});

req.on('end', function () {
res.writeHead(200, {'content-type': 'application/json'});
let response = {
'received-content-length': Buffer.concat(body).byteLength
};

res.end(JSON.stringify(response));
});
}
else {
res.writeHead(200, {'content-type': 'text/plain'});
res.end('Okay!');
}
});

module.exports = httpServer;
122 changes: 121 additions & 1 deletion test/integration/file-uploads/request-body.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
var fs = require('fs'),
expect = require('chai').expect,
sinon = require('sinon'),
IS_BROWSER = typeof window !== 'undefined';
IS_BROWSER = typeof window !== 'undefined',
Copy link
Member

Choose a reason for hiding this comment

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

Why it's not working in the browser?

{Readable} = require('stream');

describe('file upload in request body', function () {
var testrun;
Expand Down Expand Up @@ -645,4 +646,123 @@ describe('file upload in request body', function () {
.to.equal('Binary file load error: file resolver interface mismatch');
});
});

(IS_BROWSER ? describe.skip : describe)('large file upload in request body', function () {
const inStream = new Readable({
// eslint-disable-next-line no-empty-function
read () {}
});

// eslint-disable-next-line mocha/no-sibling-hooks
before(function (done) {
this.run({
// using a custom file-resolver since we don't want to create
// actual file size of 50MB for testing large file uploads
fileResolver: {
codenirvana marked this conversation as resolved.
Show resolved Hide resolved
stat: function (src, cb) {
cb(null, {isFile: function () { return true; }, mode: 33188});
},
createReadStream: function () {
Copy link
Member

Choose a reason for hiding this comment

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

Why it's returning a buffer instead of a. stream?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, returning a stream in case of file mode, when returning stream in case of form-data it is not working response is undefined in that case, hence using Buffer.alloc(). I tried reading about how form-data works but couldn't find a solution, do let me know if I am missing something here.

Copy link
Member

Choose a reason for hiding this comment

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

Because you can't re-use the same stream. Return a new stream here instead of pushing data to a common stream.

Copy link
Author

Choose a reason for hiding this comment

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

I've tried using the new stream still I get response as undefined the error has calls going in recursion will need to verify where it is failing in case of streams.

// creating buffer of size 52428800 bytes corresponds to 50 MB
inStream.push(Buffer.alloc(50 * 1024 * 1024));
inStream.push(null);

return inStream;
}
},
collection: {
item: [{
request: {
url: global.servers.http + '/upload',
method: 'POST',
body: {
mode: 'file',
codenirvana marked this conversation as resolved.
Show resolved Hide resolved
file: {src: 'test/fixtures/upload-file-large-dummy'}
}
}
}]
}
}, function (err, results) {
testrun = results;
done(err);
});
});

// eslint-disable-next-line mocha/no-identical-title
it('should complete the run', function () {
expect(testrun).to.be.ok;
sinon.assert.calledOnce(testrun.start);
sinon.assert.calledOnce(testrun.done);
sinon.assert.calledWith(testrun.done.getCall(0), null);
sinon.assert.callCount(testrun.request, 1);
});

it('should upload the large file correctly', function () {
var response = testrun.request.getCall(0).args[2];

expect(response.reason()).to.eql('OK');
// 52428800 bytes corresponds to 50 MB
expect(response.json()).to.nested.include({
'received-content-length': 52428800
});
sinon.assert.calledWith(testrun.request.getCall(0), null);
});
});

(IS_BROWSER ? describe.skip : describe)('large file upload in form-data mode', function () {
// eslint-disable-next-line mocha/no-sibling-hooks
before(function (done) {
this.run({
// using a custom file-resolver since we don't want to create
// actual file size of 50MB for testing large file uploads
fileResolver: {
stat: function (src, cb) {
cb(null, {isFile: function () { return true; }, mode: 33188});
},
createReadStream: function () {
// creating buffer of size 52428800 bytes corresponds to 50 MB
return Buffer.alloc(50 * 1024 * 1024);
}
},
collection: {
item: [{
request: {
url: global.servers.http + '/upload',
method: 'POST',
body: {
mode: 'formdata',
formdata: [{
key: 'file',
src: 'test/fixtures/upload-file-large-dummy',
type: 'file'
}]
}
}
}]
}
}, function (err, results) {
testrun = results;
done(err);
});
});

// eslint-disable-next-line mocha/no-identical-title
it('should complete the run', function () {
expect(testrun).to.be.ok;
sinon.assert.calledOnce(testrun.start);
sinon.assert.calledOnce(testrun.done);
sinon.assert.calledWith(testrun.done.getCall(0), null);
sinon.assert.callCount(testrun.request, 1);
});

it('should upload the file in formdata mode correctly', function () {
var response = testrun.request.getCall(0).args[2];

sinon.assert.calledWith(testrun.request.getCall(0), null);
expect(response.reason()).to.eql('OK');
expect(response.json()).to.nested.include({
'received-content-length': 52428999
});
});
});
});