Enabled "read with streaming" unit test for browsers that support streaming #9268

shikhar-scs · 2017-12-12T10:43:04Z

As mentioned by @timvandermeij 3 days ago, I have made the required changes, also referring to the comments mentioned by @Snuffleupagus here.

This hopefully closes #8851 .

Snuffleupagus · 2017-12-12T13:14:35Z

test/unit/network_spec.js

+      /Mozilla\/5.0.*?rv:(?:\d+).*? Gecko/.test(userAgent);
+    let isFetchWithStreamSupport = (typeof Response !== 'undefined' &&
+      'body' in Response.prototype &&
+      typeof ReadableStream !== 'undefined');


Nit: Please align these lines vertically, like so:

let isFetchWithStreamSupport = (typeof Response !== 'undefined' && 'body' in Response.prototype && typeof ReadableStream !== 'undefined');

Snuffleupagus · 2017-12-12T13:14:49Z

test/unit/network_spec.js

+      typeof ReadableStream !== 'undefined');
+
+    if (!isFirefoxWithMozChunkedEncodingSupport &&
+      !isFetchWithStreamSupport) {


Nit: Insufficient indentation on this line, please make sure that the two lines are aligned like below:

if (!isFirefoxWithMozChunkedEncodingSupport && !isFetchWithStreamSupport) {

Yeah Sure I'll do that rightway. Thanks for reviewing .

Snuffleupagus · 2017-12-12T17:53:45Z

test/unit/network_spec.js

@@ -67,11 +67,15 @@ describe('network', function() {
    // The test is valid for FF only: the XHR has support of the
    // 'moz-chunked-array' response type.
    // TODO enable for other browsers, e.g. when fetch/streams API is supported.


As mentioned in https://github.com/mozilla/pdf.js/pull/9050/files/#r154503634, these comments are no longer necessary.

Snuffleupagus · 2017-12-12T17:54:16Z

test/unit/network_spec.js

+    var isFirefoxWithMozChunkedEncodingSupport =
+      /Mozilla\/5.0.*?rv:(?:\d+).*? Gecko/.test(userAgent);
+    let isFetchWithStreamSupport = (typeof Response !== 'undefined' &&
+                                     'body' in Response.prototype &&


There's one space too many on this line, please remove it.

Finally, you need to squash the commits when addressing review feedback; please see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

timvandermeij · 2017-12-12T20:53:30Z

test/unit/network_spec.js

-      expect(true).toEqual(true);
-      done();
-      return;
+    var isFirefoxWithMozChunkedEncodingSupport =


Change var to let here too.

timvandermeij · 2017-12-12T20:55:28Z

Please squash the commits into one commit after you've made these changes. Refer to https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for how to do that easily.

improved indentation changes done as required

shikhar-scs · 2017-12-13T02:08:34Z

@timvandermeij
Sir I have changed that var to let and the commits have been squashed too.
@Snuffleupagus
Sir, that extra white space was removed and unnecessary comments have been removed.

Snuffleupagus · 2017-12-13T08:45:17Z

/botio unittest

pdfjsbot · 2017-12-13T08:45:18Z

From: Bot.io (Windows)

Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/28ef1909f36ab6a/output.txt

pdfjsbot · 2017-12-13T08:45:18Z

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/23352ba67bd399d/output.txt

pdfjsbot · 2017-12-13T08:48:14Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/23352ba67bd399d/output.txt

Total script time: 2.92 mins

Unit Tests: FAILED

pdfjsbot · 2017-12-13T08:51:52Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/28ef1909f36ab6a/output.txt

Total script time: 6.56 mins

Unit Tests: FAILED

shikhar-scs · 2017-12-13T09:17:35Z

@Snuffleupagus Why is the unit tests failing in either of the OS?

TEST-UNEXPECTED-FAIL | read with streaming | Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

Snuffleupagus · 2017-12-13T09:22:40Z

Why is the unit tests failing in either of the OS?

I don't know offhand, but they seem to fail locally as well. Did you successfully run the unit-tests locally with this patch?

shikhar-scs · 2017-12-13T09:39:04Z

Initially while commiting I did and it was working perfectly. However after squashing i didnt perform the unit test.

Also I tried commiting again in order to check if any difference has been made in any other file. However,
that was not the case.

On branch enable-read-with-stream
nothing to commit, working directory clean

Running gulp unittest now renders this

gulp unittest
[15:01:32] Using gulpfile ~/Desktop/mozillaPdf/pdf.js/gulpfile.js
[15:01:32] Starting 'buildnumber'...

Getting extension build number

[15:01:32] Starting 'locale'...

Building localization files

Extension build number: 204
[15:01:32] Finished 'buildnumber' after 232 ms
[15:01:32] Starting 'components'...

Creating generic components

[15:01:37] Version: webpack 3.10.0
Asset Size Chunks Chunk Names
pdf_viewer.js 173 kB 0 [emitted] main
pdf_viewer.js.map 272 kB 0 [emitted] main
[15:01:37] Finished 'components' after 4.83 s
[15:01:37] Finished 'locale' after 5.2 s
[15:01:37] Starting 'generic'...

Creating generic viewer

Bundling files into pdf.js

[15:01:52] Version: webpack 3.10.0
Asset Size Chunks Chunk Names
viewer.js 364 kB 0 [emitted] [big] main
viewer.js.map 580 kB 0 [emitted] main
[15:02:14] Version: webpack 3.10.0
Asset Size Chunks Chunk Names
pdf.js 601 kB 0 [emitted] [big] main
pdf.js.map 989 kB 0 [emitted] main
[15:02:16] Version: webpack 3.10.0
Asset Size Chunks Chunk Names
pdf.worker.js 1.5 MB 0 [emitted] [big] main
pdf.worker.js.map 2.77 MB 0 [emitted] main
[15:02:16] Finished 'generic' after 39 s
[15:02:16] Starting 'unittest'...

Running unit tests

WARNING: File was not downloaded. See "pdfs/bug951051.pdf.error" file.
Unable to verify the checksum for the files that are used for testing.
Please re-download the files, or adjust the MD5 checksum in the manifest for the files listed above.

Server running at http://127.0.0.1:43871/
fs.js:1649
binding.lstat(baseLong);
^

Error: ENOENT: no such file or directory, lstat '/path'
at Object.realpathSync (fs.js:1649:15)
at Function.WebBrowser.create (/home/shikhar/Desktop/mozillaPdf/pdf.js/test/webbrowser.js:250:17)
at /home/shikhar/Desktop/mozillaPdf/pdf.js/test/test.js:643:30
at Array.forEach ()
at startBrowsers (/home/shikhar/Desktop/mozillaPdf/pdf.js/test/test.js:642:12)
at startUnitTest (/home/shikhar/Desktop/mozillaPdf/pdf.js/test/test.js:564:3)
at /home/shikhar/Desktop/mozillaPdf/pdf.js/test/test.js:734:7
at /home/shikhar/Desktop/mozillaPdf/pdf.js/test/test.js:718:7
at verifyNext (/home/shikhar/Desktop/mozillaPdf/pdf.js/test/downloadutils.js:151:7)
at /home/shikhar/Desktop/mozillaPdf/pdf.js/test/downloadutils.js:178:7
[15:02:18] Finished 'unittest' after 1.48 s

Also I tried commiting again in order to check if any difference has been made in any other file. However,
that was not the case.

shikhar-scs · 2017-12-13T10:27:32Z

@Snuffleupagus An HTTP 504 error gets generated at "pdfs/bug951051.pdf.error" in the test folder.

shikhar-scs · 2017-12-13T11:07:23Z

The following snippet was picked from the previous long text only.

WARNING: File was not downloaded. See "pdfs/bug951051.pdf.error" file.
Unable to verify the checksum for the files that are used for testing.
Please re-download the files, or adjust the MD5 checksum in the manifest for the files listed above.

(sorry I dont know how to add a specific reply text)

@Snuffleupagus
I tried re downloading the file and ran the test again but to no avail. The same errors are repeating

Snuffleupagus · 2017-12-13T12:08:55Z

I'm not sure if the errors mentioned above are that relevant to the unit tests that we're concerned with here.

However, now that I look at the code in https://github.com/mozilla/pdf.js/blob/master/src/display/network.js, I'm not really sure how this unit-test would work in non-Firefox browsers, given that the fetch functionality is implemented in a different file; see https://github.com/mozilla/pdf.js/blob/master/src/display/fetch_stream.js.

Since the unit-tests in the network_spec.js file are only intended to test functionality in the network.js file, I now quite confused as to how this could actually work!?

@timvandermeij Any ideas here?

shikhar-scs · 2017-12-13T15:05:52Z

@Snuffleupagus I guess this new error that has occurred remained hidden initially because of the error which was reported in issue #8851 . Now that #8851 is corrected (hopefully) this error re-surfaced (which it originally should have had on its own).
I might be wrong though.

timvandermeij · 2017-12-13T21:15:53Z

The fetch functionality is indeed in a different file and ideally should also be unit tested in a different file for clarity. Looking at this particular test, it looks like it assumes the fetch functionality to work and rather focus on testing the read functionality of PDFNetworkStream, which is fine because there are different cases in which that should work (e.g., with and without streaming). I'm not sure though why the current patch wouldn't work.

@Rob--W Could you chime in here?

shikhar-scs · 2017-12-14T10:54:54Z

Any updates?

Rob--W · 2017-12-14T23:17:31Z

When I run the tests with this patch, it passes in Firefox but faiils in Chrome.

To help with debugging, run only this test, by starting a server with gulp server and then opening the following URL in Chrome: http://127.0.0.1:8888/test/unit/unit_test.html?spec=network%20read%20with%20streaming
Then you can edit the test and refresh the page to quickly see the effect of modifying test/unit/network_spec.js

I put a console.log after

pdf.js/test/unit/network_spec.js

Line 42 in 546cd2b

return fullReader.read().then(function (result) {

and the log message is never being shown. This means that the promise returned by getFullReader().read() never resolves.

That promise is created here:

pdf.js/src/display/network.js

Lines 459 to 461 in 546cd2b

    
           var requestCapability = createPromiseCapability(); 
        
           this._requests.push(requestCapability); 
        
           return requestCapability.promise;

If I search in that file, the only location where this promise is fulfilled is at:

pdf.js/src/display/network.js

Lines 387 to 391 in 546cd2b

    
           _onProgressiveData: 
        
               function PDFNetworkStreamFullRequestReader_onProgressiveData(chunk) { 
        
             if (this._requests.length > 0) { 
        
               var requestCapability = this._requests.shift(); 
        
               requestCapability.resolve({ value: chunk, done: false, });

And onProgressiveData is only called in _onDone (= args.onDone) and in args.onProgressiveData:

pdf.js/src/display/network.js

Lines 315 to 322 in 546cd2b

    
           function PDFNetworkStreamFullRequestReader(manager, source) { 
        
             this._manager = manager; 
        
             var args = { 
        
               onHeadersReceived: this._onHeadersReceived.bind(this), 
        
               onProgressiveData: source.disableStream ? null : 
        
                                  this._onProgressiveData.bind(this), 
        
               onDone: this._onDone.bind(this),

These functions are used in NetworkManager (which contains logic that is conditional on mozChunked - note that requestFull in the next snippet is called by )PDFNetworkStreamFullRequestReader:

pdf.js/src/display/network.js

Lines 86 to 139 in 546cd2b

    
           requestFull: function NetworkManager_requestFull(listeners) { 
        
             return this.request(listeners); 
        
           }, 
        
           request: function NetworkManager_request(args) { 
        
             var xhr = this.getXhr(); 
        
             var xhrId = this.currXhrId++; 
        
             var pendingRequest = this.pendingRequests[xhrId] = { 
        
               xhr, 
        
             }; 
        
             xhr.open('GET', this.url); 
        
             xhr.withCredentials = this.withCredentials; 
        
             for (var property in this.httpHeaders) { 
        
               var value = this.httpHeaders[property]; 
        
               if (typeof value === 'undefined') { 
        
                 continue; 
        
               } 
        
               xhr.setRequestHeader(property, value); 
        
             } 
        
             if (this.isHttp && 'begin' in args && 'end' in args) { 
        
               var rangeStr = args.begin + '-' + (args.end - 1); 
        
               xhr.setRequestHeader('Range', 'bytes=' + rangeStr); 
        
               pendingRequest.expectedStatus = 206; 
        
             } else { 
        
               pendingRequest.expectedStatus = 200; 
        
             } 
        
             var useMozChunkedLoading = supportsMozChunked && !!args.onProgressiveData; 
        
             if (useMozChunkedLoading) { 
        
               xhr.responseType = 'moz-chunked-arraybuffer'; 
        
               pendingRequest.onProgressiveData = args.onProgressiveData; 
        
               pendingRequest.mozChunked = true; 
        
             } else { 
        
               xhr.responseType = 'arraybuffer'; 
        
             } 
        
             if (args.onError) { 
        
               xhr.onerror = function(evt) { 
        
                 args.onError(xhr.status); 
        
               }; 
        
             } 
        
             xhr.onreadystatechange = this.onStateChange.bind(this, xhrId); 
        
             xhr.onprogress = this.onProgress.bind(this, xhrId); 
        
             pendingRequest.onHeadersReceived = args.onHeadersReceived; 
        
             pendingRequest.onDone = args.onDone; 
        
             pendingRequest.onError = args.onError; 
        
             pendingRequest.onProgress = args.onProgress; 
        
             xhr.send(null); 
        
             return xhrId; 
        
           },

I stopped looking further, because I was expecting the test to use the fetch API in Chrome, and the above snippet clearly shows that XMLHttpRequest is used, which does not support streaming responses.

In Chrome, the following logic should have been called in the test, in order to test streaming.

pdf.js/src/display/fetch_stream.js

Lines 44 to 48 in 546cd2b

    
           getFullReader() { 
        
             assert(!this._fullRequestReader); 
        
             this._fullRequestReader = new PDFFetchStreamReader(this); 
        
             return this._fullRequestReader; 
        
           }

@yurydelendik wrote this unit test. Why is the test unconditionally using the PDFNetworkStream from src/display/network.js? (instead of feature-detecting the most appropriate IPDFStream implementation and testing that?)

mukulmishra18 · 2017-12-15T06:46:12Z

Why is the test unconditionally using the PDFNetworkStream from src/display/network.js? (instead of feature-detecting the most appropriate IPDFStream implementation and testing that?)

You are right. We are doing wrong here. We should detect for the support of fetch/stream and based on that set PDFNetworkStream just like we are doing here:

pdf.js/src/pdf.js

Lines 34 to 44 in 546cd2b

    
           if (pdfjsSharedUtil.isNodeJS()) { 
        
             var PDFNodeStream = require('./display/node_stream.js').PDFNodeStream; 
        
             pdfjsDisplayAPI.setPDFNetworkStreamClass(PDFNodeStream); 
        
           } else if (typeof Response !== 'undefined' && 'body' in Response.prototype && 
        
                      typeof ReadableStream !== 'undefined') { 
        
             var PDFFetchStream = require('./display/fetch_stream.js').PDFFetchStream; 
        
             pdfjsDisplayAPI.setPDFNetworkStreamClass(PDFFetchStream); 
        
            } else { 
        
             var PDFNetworkStream = require('./display/network.js').PDFNetworkStream; 
        
             pdfjsDisplayAPI.setPDFNetworkStreamClass(PDFNetworkStream); 
        
           }

If you look into jasmine_boot.js file:

pdf.js/test/unit/jasmine-boot.js

Lines 83 to 89 in 546cd2b

    
           // Set network stream class for unit tests. 
        
           if (typeof Response !== 'undefined' && 'body' in Response.prototype && 
        
               typeof ReadableStream !== 'undefined') { 
        
             displayApi.setPDFNetworkStreamClass(PDFFetchStream); 
        
           } else { 
        
             displayApi.setPDFNetworkStreamClass(PDFNetworkStream); 
        
           }

we are setting PDFNetworkStream by feature-detection, but it is only applicable for test/unit/api_spec.js

shikhar-scs · 2017-12-16T07:00:18Z

@Snuffleupagus any updates?

Snuffleupagus · 2017-12-16T13:02:33Z

@Snuffleupagus any updates?

Please read #9268 (comment) and #9268 (comment) above.

The gist of it though, is that the code that we're attempting to test here isn't implemented that way was initially assumed. Hence we need to figure out how to reconcile this first, probably in a separate issue/PR, before these unit-test can be enabled. So currently, it seems to me that this PR unfortunately needs to be put on hold for the time being.

shikhar-scs · 2017-12-16T13:45:01Z

@Snuffleupagus Uhm Okay.

Rob--W · 2017-12-17T10:45:42Z

Please read #9268 (comment) and #9268 (comment) above.

The gist of it though, is that the code that we're attempting to test here isn't implemented that way was initially assumed. Hence we need to figure out how to reconcile this first,

I would start with ensuring that setPDFNetworkStreamClass (as done in jasmine-boot.js) has run in all unit tests (or at least network_spec.js). This is fairly straightforward.

timvandermeij · 2018-02-18T15:06:27Z

Closing since the change is not in a mergable state at the moment and there has been no more follow-up. The issue will remain open and new pull requests to address the issue are welcome. For anyone willing to work on this, I suggest to read the comments above and make the necessary changes in small chunks (perhaps multiple pull requests). Thanks.

shikhar-scs · 2018-02-18T15:19:36Z

😞

shikhar-scs changed the title ~~enable read with stream~~ Enabled reading with streaming Dec 12, 2017

shikhar-scs changed the title ~~Enabled reading with streaming~~ Enabled "read with streaming" unit test for browsers that support streaming Dec 12, 2017

Snuffleupagus reviewed Dec 12, 2017

View reviewed changes

timvandermeij reviewed Dec 12, 2017

View reviewed changes

timvandermeij mentioned this pull request Dec 12, 2017

Enable "read with streaming" unit test for browsers that support streaming #9050

Closed

timvandermeij added the test label Dec 12, 2017

enable read with stream

2626cf0

improved indentation changes done as required

shikhar-scs force-pushed the enable-read-with-stream branch from f236ad2 to 2626cf0 Compare December 13, 2017 02:02

timvandermeij closed this Feb 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled "read with streaming" unit test for browsers that support streaming #9268

Enabled "read with streaming" unit test for browsers that support streaming #9268

shikhar-scs commented Dec 12, 2017 •

edited

Loading

Snuffleupagus Dec 12, 2017

Snuffleupagus Dec 12, 2017

shikhar-scs Dec 12, 2017

Snuffleupagus Dec 12, 2017

Snuffleupagus Dec 12, 2017

timvandermeij Dec 12, 2017

timvandermeij commented Dec 12, 2017

shikhar-scs commented Dec 13, 2017

Snuffleupagus commented Dec 13, 2017

pdfjsbot commented Dec 13, 2017

pdfjsbot commented Dec 13, 2017

pdfjsbot commented Dec 13, 2017

pdfjsbot commented Dec 13, 2017

shikhar-scs commented Dec 13, 2017

Snuffleupagus commented Dec 13, 2017

shikhar-scs commented Dec 13, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017 •

edited

Loading

Snuffleupagus commented Dec 13, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017

timvandermeij commented Dec 13, 2017

shikhar-scs commented Dec 14, 2017

Rob--W commented Dec 14, 2017

mukulmishra18 commented Dec 15, 2017

shikhar-scs commented Dec 16, 2017

Snuffleupagus commented Dec 16, 2017

shikhar-scs commented Dec 16, 2017

Rob--W commented Dec 17, 2017

timvandermeij commented Feb 18, 2018

shikhar-scs commented Feb 18, 2018

Enabled "read with streaming" unit test for browsers that support streaming #9268

Enabled "read with streaming" unit test for browsers that support streaming #9268

Conversation

shikhar-scs commented Dec 12, 2017 • edited Loading

Snuffleupagus Dec 12, 2017

Choose a reason for hiding this comment

Snuffleupagus Dec 12, 2017

Choose a reason for hiding this comment

shikhar-scs Dec 12, 2017

Choose a reason for hiding this comment

Snuffleupagus Dec 12, 2017

Choose a reason for hiding this comment

Snuffleupagus Dec 12, 2017

Choose a reason for hiding this comment

timvandermeij Dec 12, 2017

Choose a reason for hiding this comment

timvandermeij commented Dec 12, 2017

shikhar-scs commented Dec 13, 2017

Snuffleupagus commented Dec 13, 2017

pdfjsbot commented Dec 13, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 13, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Dec 13, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Dec 13, 2017

From: Bot.io (Windows)

Failed

shikhar-scs commented Dec 13, 2017

Snuffleupagus commented Dec 13, 2017

shikhar-scs commented Dec 13, 2017 • edited Loading

Getting extension build number

Building localization files

Creating generic components

Creating generic viewer

Bundling files into pdf.js

Running unit tests

shikhar-scs commented Dec 13, 2017 • edited Loading

shikhar-scs commented Dec 13, 2017 • edited Loading

Snuffleupagus commented Dec 13, 2017 • edited Loading

shikhar-scs commented Dec 13, 2017

timvandermeij commented Dec 13, 2017

shikhar-scs commented Dec 14, 2017

Rob--W commented Dec 14, 2017

mukulmishra18 commented Dec 15, 2017

shikhar-scs commented Dec 16, 2017

Snuffleupagus commented Dec 16, 2017

shikhar-scs commented Dec 16, 2017

Rob--W commented Dec 17, 2017

timvandermeij commented Feb 18, 2018

shikhar-scs commented Feb 18, 2018

shikhar-scs commented Dec 12, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017 •

edited

Loading

shikhar-scs commented Dec 13, 2017 •

edited

Loading

Snuffleupagus commented Dec 13, 2017 •

edited

Loading