Add integration test for large file upload #1094

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

Jump to bottom

Open

lucifercr07 wants to merge 1 commit into develop

from feature/add_integration_test_for_large_file_upload

Open

Add integration test for large file upload #1094

lucifercr07 wants to merge 1 commit into develop from feature/add_integration_test_for_large_file_upload

Conversation

lucifercr07

Copy link

@lucifercr07 lucifercr07 commented Oct 5, 2020

Added integration test for file upload of 50 MB.

@lucifercr07 lucifercr07 requested review from codenirvana and coditva

October 5, 2020 07:11

@codecov

Copy link

codecov bot commented Oct 5, 2020 •

edited

Loading

Codecov Report

Merging #1094 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@
## develop #1094 +/- ##
========================================
 Coverage 91.10% 91.10% 
========================================
 Files 42 42 
 Lines 2563 2563 
 Branches 735 735 
========================================
 Hits 2335 2335 
 Misses 228 228

Flag	Coverage Δ
#integration	`79.63% <ø> (ø)`
#legacy	`56.10% <ø> (ø)`
#unit	`49.27% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebbf184...9862d37. Read the comment docs.

@lucifercr07 lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch from c3b92c4 to 44a3088 Compare

October 5, 2020 08:20

coditva

coditva requested changes

Oct 5, 2020

View reviewed changes

test/integration/file-uploads/request-body.test.js Outdated Show resolved Hide resolved

test/integration/file-uploads/request-body.test.js Show resolved Hide resolved

coditva

coditva requested changes

Oct 6, 2020

View reviewed changes

test/integration/file-uploads/request-body.test.js Outdated Show resolved Hide resolved

@lucifercr07 lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch 3 times, most recently from 0c90ac5 to 77162d1 Compare

October 6, 2020 13:58

codenirvana

codenirvana requested changes

Oct 7, 2020

View reviewed changes

Copy link

Member

@codenirvana codenirvana left a comment

Choose a reason for hiding this comment

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

Let's see if we can achieve this by overriding the file resolver and sending a dynamic read stream.

codenirvana

codenirvana requested changes

Oct 13, 2020

View reviewed changes

CHANGELOG.yaml Outdated Show resolved Hide resolved

test/fixtures/servers/http.js Outdated Show resolved Hide resolved

test/integration/file-uploads/request-body.test.js Outdated Show resolved Hide resolved

test/integration/file-uploads/request-body.test.js

stat: function (src, cb) {

cb(null, {isFile: function () { return true; }, mode: 33188});

createReadStream: function () {

Copy link

Member

@codenirvana codenirvana Oct 13, 2020

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

@lucifercr07 lucifercr07 Oct 20, 2020

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

@codenirvana codenirvana Oct 21, 2020

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

@lucifercr07 lucifercr07 Oct 21, 2020

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.

test/integration/file-uploads/request-body.test.js Show resolved Hide resolved

@lucifercr07 lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch 2 times, most recently from d6f5c19 to 5d2b9d4 Compare

October 20, 2020 21:24

codenirvana

codenirvana requested changes

Oct 21, 2020

View reviewed changes

Copy link

Member

@codenirvana codenirvana left a comment

Choose a reason for hiding this comment

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

Also, move the integration test to /test/integration/benchmark.

CHANGELOG.yaml Outdated

@@ -1,3 +1,7 @@

master:

chores:

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

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

wrong indentation.

test/integration/file-uploads/request-body.test.js

expect = require('chai').expect,

sinon = require('sinon'),

IS_BROWSER = typeof window !== 'undefined';

IS_BROWSER = typeof window !== 'undefined',

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

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?

test/fixtures/servers/http.js

});

httpServer.on('/file-upload', function (req, res) {

if (req.method === 'POST') {

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

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

@lucifercr07 lucifercr07 Oct 21, 2020

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

@codenirvana codenirvana Oct 27, 2020

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.

test/fixtures/servers/http.js

let body = [];

req.on('data', function (data) {

body.push(data);

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

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

@lucifercr07 lucifercr07 Oct 21, 2020

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

@codenirvana codenirvana Oct 27, 2020

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.

test/fixtures/servers/http.js Outdated

res.end();

});

httpServer.on('/file-upload', function (req, res) {

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

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

Since it's not parsing any file, this can just be /upload.

test/fixtures/servers/http.js Outdated

req.on('end', function () {

res.writeHead(200, {'content-type': 'text/plain'});

res.end('received-content-length:' + Buffer.concat(body).byteLength);

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

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

What kind of an API response is this?
Either return the bytes received or a JSON response.

Copy link

Author

@lucifercr07 lucifercr07 Oct 21, 2020

Choose a reason for hiding this comment

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

Was returning a plain-text response similar to what is being done in Line 38 of this file, do let me know if I need to change that as well.

test/integration/file-uploads/request-body.test.js

stat: function (src, cb) {

cb(null, {isFile: function () { return true; }, mode: 33188});

createReadStream: function () {

Copy link

Member

@codenirvana codenirvana Oct 21, 2020

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.


 Add integration test for large file upload

9862d37

Added integration test for file upload of 50 MB.

@lucifercr07 lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch from 5d2b9d4 to 9862d37 Compare

October 21, 2020 12:06

@codenirvana codenirvana force-pushed the develop branch from b5fc76a to 2717d97 Compare

April 2, 2024 05:19

Labels

None yet

3 participants

@lucifercr07 @codenirvana @coditva

Add integration test for large file upload #1094

Are you sure you want to change the base?

Add integration test for large file upload #1094

Uh oh!

Conversation

@lucifercr07 lucifercr07 commented Oct 5, 2020

Uh oh!

codecov bot commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@codenirvana codenirvana left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@codenirvana codenirvana Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

@lucifercr07 lucifercr07 Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@lucifercr07 lucifercr07 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@codenirvana codenirvana left a comment

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@lucifercr07 lucifercr07 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@lucifercr07 lucifercr07 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@lucifercr07 lucifercr07 Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

@codenirvana codenirvana Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

codecov bot commented Oct 5, 2020 •

edited

Loading