Add integration test for large file upload #1094

lucifercr07 · 2020-10-05T07:11:45Z

Added integration test for file upload of 50 MB.

codecov · 2020-10-05T07:13:53Z

Codecov Report

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

@@           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.

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

codenirvana

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

CHANGELOG.yaml

test/fixtures/servers/http.js

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

codenirvana · 2020-10-13T10:35:58Z

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

+                    stat: function (src, cb) {
+                        cb(null, {isFile: function () { return true; }, mode: 33188});
+                    },
+                    createReadStream: function () {


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

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.

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

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

codenirvana

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

codenirvana · 2020-10-21T05:11:34Z

CHANGELOG.yaml

@@ -1,3 +1,7 @@
+master:
+    chores:


wrong indentation.

codenirvana · 2020-10-21T05:15:23Z

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

@@ -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',


Why it's not working in the browser?

codenirvana · 2020-10-21T05:16:55Z

test/fixtures/servers/http.js

@@ -48,4 +48,19 @@ httpServer.on('/custom-reason', function (req, res) {
    res.end();
 });

+httpServer.on('/file-upload', function (req, res) {
+    if (req.method === 'POST') {


Why it only supports POST?

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

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

codenirvana · 2020-10-21T05:17:54Z

test/fixtures/servers/http.js

+        let body = [];
+
+        req.on('data', function (data) {
+            body.push(data);


Buffer has a concat method.

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.

Have you benchmarked? Share the results.

codenirvana · 2020-10-21T05:22:47Z

test/fixtures/servers/http.js

@@ -48,4 +48,19 @@ httpServer.on('/custom-reason', function (req, res) {
    res.end();
 });

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


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

codenirvana · 2020-10-21T05:24:16Z

test/fixtures/servers/http.js

+
+        req.on('end', function () {
+            res.writeHead(200, {'content-type': 'text/plain'});
+            res.end('received-content-length:' + Buffer.concat(body).byteLength);


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

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.

codenirvana · 2020-10-21T05:26:43Z

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

+                    stat: function (src, cb) {
+                        cb(null, {isFile: function () { return true; }, mode: 33188});
+                    },
+                    createReadStream: function () {


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

Added integration test for file upload of 50 MB.

lucifercr07 requested review from codenirvana and coditva October 5, 2020 07:11

lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch from c3b92c4 to 44a3088 Compare October 5, 2020 08:20

coditva requested changes Oct 5, 2020

View reviewed changes

coditva requested changes Oct 6, 2020

View reviewed changes

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

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 requested changes Oct 7, 2020

View reviewed changes

codenirvana requested changes Oct 13, 2020

View reviewed changes

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 requested changes Oct 21, 2020

View reviewed changes

Add integration test for large file upload

9862d37

Added integration test for file upload of 50 MB.

lucifercr07 force-pushed the feature/add_integration_test_for_large_file_upload branch from 5d2b9d4 to 9862d37 Compare October 21, 2020 12:06

codenirvana force-pushed the develop branch from b5fc76a to 2717d97 Compare April 2, 2024 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test for large file upload #1094

Add integration test for large file upload #1094

lucifercr07 commented Oct 5, 2020

codecov bot commented Oct 5, 2020 •

edited

Loading

codenirvana left a comment

codenirvana Oct 13, 2020

lucifercr07 Oct 20, 2020

codenirvana Oct 21, 2020

lucifercr07 Oct 21, 2020

codenirvana left a comment

codenirvana Oct 21, 2020

codenirvana Oct 21, 2020

codenirvana Oct 21, 2020

lucifercr07 Oct 21, 2020

codenirvana Oct 27, 2020

codenirvana Oct 21, 2020

lucifercr07 Oct 21, 2020

codenirvana Oct 27, 2020

codenirvana Oct 21, 2020

codenirvana Oct 21, 2020

lucifercr07 Oct 21, 2020

codenirvana Oct 21, 2020

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

Conversation

lucifercr07 commented Oct 5, 2020

codecov bot commented Oct 5, 2020 • edited Loading

Codecov Report

codenirvana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenirvana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 5, 2020 •

edited

Loading