enabled coverage reports for browsers Solves #8632 #9308

shikhar-scs · 2017-12-22T05:03:54Z

As widely discussed here, I have made all the necessary changes and now coverage reports are getting generated finally, in the build/lib/coverage/lcov-report folder.

npm run coveralls can be used to generate the same . Also they are re generated after every successful build

Also due to conflicts in ES5 and ES6 they had to be based on the build lib folder only.

This solves #8632.

@Snuffleupagus @timvandermeij @yurydelendik please review this.

Here is a link for viewing the reports pdfjscoveragereport.bitballoon.com

removed gemfile.lock for build introduced jasmine coverage coverage reports are now getting generated replaced coveralls target replaced coveralls targets trying coverall.io again might work I hope might work I hope- coveralls working fine coveralls is working fine Delete .idea files Delete .idea Delete .idea files Delete .idea files Delete .idea Delete .idea Delete .idea managing idea folder

Snuffleupagus

@Snuffleupagus @timvandermeij @yurydelendik please review this.

Personally I've got no experience with generating coverage reports, and consequently I'm not able to in any meaningful way help review this unfortunately.
The issue was filed by @Rob--W, and based on #8632 (comment) it seems that he's actually got experience with test-coverage reports. Hence it'd be great if he's got time to help do an in-depth review here.

Finally, one small note about the commit message: Please remove all the unrelated comments from it, i.e. lines such as "might work I hope", "delete ...", "removed ...", and other unrelated information.

Snuffleupagus · 2017-12-22T10:14:51Z

.gitignore

@@ -8,3 +8,4 @@ node_modules/
 examples/node/svgdump/
 examples/node/pdf2png/*.png
 package-lock.json
+.idea


This shouldn't be here, please remove it.

Sure will do that in my next commit

This line is still here (don't forget to remove the new trailing line too).

shikhar-scs · 2017-12-22T10:39:06Z

Personally I've got no experience with generating coverage reports, and consequently I'm not able to in any meaningful way help review this unfortunately.

No problem @Snuffleupagus Ill wait for @Rob--W

Finally, one small note about the commit message: Please remove all the unrelated comments from it, i.e. lines such as "might work I hope", "delete ...", "removed ...", and other unrelated information.

Yup sure I'll do that . That actually is a result of squashing

Rob--W · 2017-12-22T11:33:04Z

.coveralls.yml

@@ -0,0 +1,2 @@
+repo_token: j0CpLqyVHdQZBFScn3qznXi3mY5uGuro4


This whole file should be removed. The repo_token in particular should be treated as a secret (and will be configured via an environment variable when coveralls is enabled for PDF.js's main repo).

From https://coveralls.zendesk.com/hc/en-us/articles/201347419-Coveralls-currently-supports :

The option repo_token (found on your repository's page on Coveralls) is used to specify which project on Coveralls your project maps to. This is only needed for private repos and should be kept secret -- anyone could use it to submit coverage data on your repo's behalf.

@Rob--W I'll keep that in mind.

Rob--W · 2017-12-22T11:33:40Z

package.json

    "jasmine": "^2.8.0",
    "jasmine-core": "^2.8.0",
    "jsdoc": "^3.5.5",
    "merge-stream": "^1.0.1",
    "mkdirp": "^0.5.1",
+    "mocha-lcov-reporter": "^1.3.0",


Unused dependency - remove this.

Rob--W · 2017-12-22T11:33:48Z

package.json

+        }
+      ]
+    ]
+  },


Unused code - remove the whole browserify block.

Rob--W · 2017-12-22T11:36:33Z

package.json

  "scripts": {
-    "test": "gulp lint unittestcli externaltest"
+    "test": "gulp lint unittestcli externaltest",
+    "cover": "cd build && cd lib && istanbul cover --include-all-sources jasmine-node test",


jasmine-node has not been maintained for three years. It is also unnecessary, since the main jasmine project also supports Node.js.

Have you tried to use jasmine instead of jasmine here?

didn't know that. I'll try and use jasmine here instead.

Rob--W · 2017-12-22T11:37:31Z

package.json

+    "babel-preset-stage-0": "^6.24.1",
+    "babel-register": "^6.26.0",
+    "jasmine-node": "^1.14.5"
+  }


None of these dependencies are used - remove them. In the future, if you add new dependencies, store them in the devDependencies dictionary.

Rob--W · 2017-12-22T12:07:19Z

.babelrc

+      "plugins": [ "istanbul" ]
+    }
+  }
+}


Is this file used? I don't think so, so let's remove it.

Rob--W · 2017-12-22T12:21:51Z

Thanks for starting this experiment and providing an example of output - this is very helpful in evaluating the effectiveness of the patch.

When you invoke cd build && cd lib && instanbul cover --include-all-sources jasmine-node test, then the following happens:

Set the current to build/lib/. This directory is generated by the gulp lib task (which itself is a dependency of the unittestcli and dist-pre tasks):

pdf.js/gulpfile.js

Lines 965 to 1037 in e081a70

    
           gulp.task('lib', ['buildnumber'], function () { 
        
             // When we create a bundle, webpack is run on the source and it will replace 
        
             // require with __webpack_require__. When we want to use the real require, 
        
             // __non_webpack_require__ has to be used. 
        
             // In this target, we don't create a bundle, so we have to replace the 
        
             // occurences of __non_webpack_require__ ourselves. 
        
             function babelPluginReplaceNonWebPackRequire(babel) { 
        
               return { 
        
                 visitor: { 
        
                   Identifier(path, state) { 
        
                     if (path.node.name === '__non_webpack_require__') { 
        
                       path.replaceWith(babel.types.identifier('require')); 
        
                     } 
        
                   }, 
        
                 }, 
        
               }; 
        
             } 
        
             function preprocess(content) { 
        
               var noPreset = /\/\*\s*no-babel-preset\s*\*\//.test(content); 
        
               content = preprocessor2.preprocessPDFJSCode(ctx, content); 
        
               content = babel.transform(content, { 
        
                 sourceType: 'module', 
        
                 presets: noPreset ? undefined : ['env'], 
        
                 plugins: [ 
        
                   'transform-es2015-modules-commonjs', 
        
                   babelPluginReplaceNonWebPackRequire, 
        
                 ], 
        
               }).code; 
        
               var removeCjsSrc = 
        
                 /^(var\s+\w+\s*=\s*require\('.*?)(?:\/src)(\/[^']*'\);)$/gm; 
        
               content = content.replace(removeCjsSrc, function (all, prefix, suffix) { 
        
                 return prefix + suffix; 
        
               }); 
        
               return licenseHeader + content; 
        
             } 
        
             var babel = require('babel-core'); 
        
             var versionInfo = getVersionJSON(); 
        
             var ctx = { 
        
               rootPath: __dirname, 
        
               saveComments: false, 
        
               defines: builder.merge(DEFINES, { 
        
                 GENERIC: true, 
        
                 LIB: true, 
        
                 BUNDLE_VERSION: versionInfo.version, 
        
                 BUNDLE_BUILD: versionInfo.commit, 
        
               }), 
        
               map: { 
        
                 'pdfjs-lib': '../pdf', 
        
               }, 
        
             }; 
        
             var licenseHeader = fs.readFileSync('./src/license_header.js').toString(); 
        
             var preprocessor2 = require('./external/builder/preprocessor2.js'); 
        
             var buildLib = merge([ 
        
               gulp.src([ 
        
                 'src/{core,display}/*.js', 
        
                 'src/shared/{compatibility,util,streams_polyfill,global_scope}.js', 
        
                 'src/{pdf,pdf.worker}.js', 
        
               ], { base: 'src/', }), 
        
               gulp.src([ 
        
                 'examples/node/domstubs.js', 
        
                 'web/*.js', 
        
                 '!web/pdfjs.js', 
        
                 '!web/viewer.js', 
        
               ], { base: '.', }), 
        
               gulp.src('test/unit/*.js', { base: '.', }), 
        
             ]).pipe(transform('utf8', preprocess)) 
        
               .pipe(gulp.dest('build/lib/')); 
        
             return merge([ 
        
               buildLib, 
        
               gulp.src('external/streams/streams-lib.js', { base: '.', }) 
        
                 .pipe(gulp.dest('build/')), 
        
             ]); 
        
           });

Add test instrumentation to all files in that directory (via istanbul cover).
Run the tests in the "test" directory via jasmine (this command is invoked by istanbul). The build/lib/test directory is also generated by the previously mentioned gulp lib task).
When the jasmine test runner finishes, coverage reports are accumulated by istanbul and written to the coverage/lcov.info.
If npm run coveralls was called (instead of npm run cover), then this lcov.info file is passed to coveralls, which uploads the result to https://coveralls.io

After running the above steps, we get test coverage for build/lib, after running the tests in build/lib/test. So we only achieve test coverage for a compiled version of lib/ (to build/lib) when we run the unit tests in the CLI (using Node.js).

There are two concerns in this set up:

The compiled library is tested, not the original source files.
It is not too difficult for humans to look up the original source code given the coverage report, so for the first prototype this is acceptable.
The instrumented code is only tested against our unit tests, not against the PDF tests.
Most of the functionality is not covered by unit tests, but by rendering PDF files and checking the result (browsertest - see Generate (test) coverage statistics #8632). The generated coverage report under-represents the extent of test coverage.

On the other hand, achieving test coverage (which means that an execution of the program has run through a part of a program's source code) does not mean that the code has actually been tested: it is possible that the code path has no impact on the rendered result. So if we somehow get 100% test coverage, that does not mean that our code base is fully covered by tests. However, if we get 70% test coverage, we know that 30% of the code base is certainly not covered by tests.

The second point is a big issue; the current coverage reports suggest that there is barely any test coverage, which is far from reality. I will add some suggestions to address this at #8632.

Rob--W · 2017-12-22T12:52:09Z

I have posted my suggestions at #8632 (comment).

Note also that if you want to start with test coverage for unit tests only, then you should consider using similar logic as the unittestcli gulp task:

pdf.js/gulpfile.js

Lines 1129 to 1140 in e081a70

    
           gulp.task('unittestcli', ['lib'], function (done) { 
        
             var args = ['JASMINE_CONFIG_PATH=test/unit/clitests.json']; 
        
             var testProcess = spawn('node_modules/.bin/jasmine', args, 
        
                                     { stdio: 'inherit', }); 
        
             testProcess.on('close', function (code) { 
        
               if (code !== 0) { 
        
                 done(new Error('Unit tests failed.')); 
        
                 return; 
        
               } 
        
               done(); 
        
             }); 
        
           });

Note that the node_modules/.bin/ prefix is not necessary in package.json, because that directory is always appended to the PATH environment variable before running the script.
The main difference between your current use of jasmine (you should have used jasmine instead of jasmine-core) is that the unittestcli target also specifies the JASMINE_CONFIG_PATH environment variable. According to Jasmine's documentation, this can also be a command line argument: https://jasmine.github.io/setup/nodejs.html#jasmine_config_path

So to get the same logic as in unittestcli, I would expect something like this to work:

instanbul cover --include-all-sources jasmine JASMINE_CONFIG_PATH=test/unit/clitests.json

shikhar-scs · 2017-12-22T12:58:53Z

Thanx a lot @Rob--W . I really appreciate mentors from mozilla like you, @timvandermeij and @Snuffleupagus who constantly take out time and help out newcomers like me. I'll surely followup all this and revert back ASAP.

shikhar-scs · 2017-12-23T06:45:48Z

@Rob--W
A suggested by you, I visited this blog here http://blog.silicak.es/2016-07-07-testing-browser-gulp-phantomJS-mocha-istanbul. And for enabling browsertests using istanbul and jasmine, what I could gather from here (and also from what you had written here So, to get browser tests, we need: A way to generate coverage data.) is we basically require these 4 steps : -

Instrumentation to a coverage/ folder with a global default variable called window.__coverage__
Inject these instrumented files to a index.html to make them ready for browser testing.
Jasmine will now run these html files simultaneously and generate coverage.json files in coverage
folder.
coverage.json to be parsed by gulp-istanbul and generate reports in any required formats.

1 and 2 was manged from the blog mentioned above, however, now, when I run gulp inject I get the following error (because of which I could not proceed further)

Failed to parse file: /home/shikhar/Desktop/mozillaPdfJs/pdf.js/src/core/annotation.js

events.js:183
      throw er; // Unhandled 'error' event
      ^
Error: Unable to parse /home/shikhar/Desktop/mozillaPdfJs/pdf.js/src/core/annotation.js

Line 16: Unexpected token

This token as you might guess is import
So, again we are getting those ES non compatibility errors.Thus, I could not proceed further.

Also, I've tried generating unittestcli reports as suggested in the comment at #8632 using the statement
jasmine JASMINE_CONFIG_PATH=test/unit/clitests.json
and this actually produces very beautiful reports for unit tests a screenshot of which is shown below. (it uses various active scripts because of which it could not be hosted locally I guess)

Another good news is that now we can run tests directly from pdf.js folder using these commands

"test": "gulp lint unittestcli externaltest",
    "cover": "istanbul cover --include-all-sources jasmine JASMINE_CONFIG_PATH=test/unit/clitests.json",
    "coveralls": "npm run cover && cat ./coverage/lcov.info | coveralls"

And indeed test reports now get generated from here itself 🎆 (which previously was not happening and thus we had to move into build > lib)
I've hosted these reports here: http://coveragereportspdfjs.bitballoon.com/
and these are the old ones http://pdfjscoveragereport.bitballoon.com/

There have been significant improvements in the percentages, however, again ES5 and ES6 problems have persisted till now and thus src folder coverage is still 0. Also, many other oflders are showing 0% coverage reports which imply browsertests are still not being checked into.

After finishing step 1, the browser tests will have a window.coverage variable (or whatever you had put in the coverageVariable configuration parameter). To get coverage reports:

Modify the test runner (https://github.com/mozilla/pdf.js/blob/e081a708c36cb2aacff7889048863723fcf23671/test/driver.js) to post the coverage result with XMLHttpRequest to the test server.
In the test server (https://github.com/mozilla/pdf.js/blob/e081a708c36cb2aacff7889048863723fcf23671/test/test.js), register a new hook to receive test results, and write that to a file using the fs Node.js API (maybe after some post-processing, such as converting it to the lcov format if needed).
Upload the report to coveralls (e.g. with the "coveralls" command as shown in #9308).

I didn't exactly get your idea of modifying the two files and being a beginner it was hard to write those files accordingly. Though if you could further guide me I might be able to do so.

I'll push all of my code and please do a review.

Overall this has been a very enriching experience and further guidance might actually help us to solve this completely.

shikhar-scs · 2017-12-23T07:12:07Z

lib/jasmine_examples/Player.js

+  this.currentlyPlayingSong.persistFavoriteStatus(true);
+};
+
+module.exports = Player;


Please ignore this file. I'll delete it in the next commit.

shikhar-scs · 2017-12-23T07:12:25Z

lib/jasmine_examples/Song.js

+  throw new Error("not yet implemented");
+};
+
+module.exports = Song;


ignore this file too

shikhar-scs · 2017-12-23T07:13:00Z

spec/helpers/jasmine_examples/SpecHelper.js

+      };
+    }
+  });
+});


shikhar-scs · 2017-12-23T07:13:10Z

spec/jasmine_examples/PlayerSpec.js

+      }).toThrowError("song is already playing");
+    });
+  });
+});


shikhar-scs · 2017-12-23T07:13:25Z

spec/support/jasmine.json

+  ],
+  "stopSpecOnExpectationFailure": false,
+  "random": false
+}


shikhar-scs · 2017-12-23T07:14:48Z

primarily, changes have been made in package.json and gulpfile.js

Rob--W

It seems that you have copied and modified some code, but I'm not sure if you understand how code instrumentation works, so I'll explain a bit more in a comment at #8632 (and also offer more concrete advice regarding the communication between the test driver and the test server).

Rob--W · 2017-12-24T13:02:53Z

.gitignore

@@ -8,3 +8,4 @@ node_modules/
 examples/node/svgdump/
 examples/node/pdf2png/*.png
 package-lock.json
+.idea


This line is still here (don't forget to remove the new trailing line too).

Rob--W · 2017-12-24T13:03:12Z

gulpfile.js

@@ -84,6 +86,11 @@ var DEFINES = {
  SKIP_BABEL: false,
 };

+var ISTANBUL_PATHS = {
+  'javascript': ['coverage/**/*.js'],


Remove the single quotes around 'javascript'.

Rob--W · 2017-12-24T13:04:18Z

gulpfile.js

@@ -1168,6 +1175,29 @@ gulp.task('lint', function (done) {
  });
 });

+gulp.task('instrument', function () {
+  return gulp.src(['src/**/*.js'])
+  // Covering files


I don't understand the purpose of this comment. Could you either remove it or rephrase it? And if you choose to keep the comment, please add space before the line so that it is aligned consistently with the following lines.

Rob--W · 2017-12-24T13:04:59Z

package.json

  },
  "repository": {
    "type": "git",
    "url": "git://github.com/mozilla/pdf.js.git"
  },
-  "license": "Apache-2.0"
+  "license": "Apache-2.0",
+  "dependencies": {


Move the dependencies below to devDependencies.

@Rob--W Thanx for all these reviews, I'll make all the changes accordingly.

Rob--W · 2017-12-24T14:53:00Z

gulpfile.js

+  return gulp.src(['src/**/*.js'])
+  // Covering files
+    .pipe(istanbul({ coverageVariable: '__coverage__',
+    }))


This formatting looks a bit weird. Can you merge it with the previous line?

Rob--W · 2017-12-24T15:02:12Z

gulpfile.js

@@ -1168,6 +1175,29 @@ gulp.task('lint', function (done) {
  });
 });

+gulp.task('instrument', function () {
+  return gulp.src(['src/**/*.js'])


Why are you using ISTANBUL_PATHS elsewhere and an array here?

Rob--W · 2017-12-24T15:03:56Z

gulpfile.js

+    .pipe(gulp.dest('coverage/'));
+});
+
+gulp.task('inject', ['instrument'], function (cb) {


Rename cb to done and actually call the callback when the task completes.

Rob--W · 2017-12-24T15:13:10Z

gulpfile.js

+});
+
+gulp.task('inject', ['instrument'], function (cb) {
+  return gulp.src('index.html')


This whole task is not doing anything because there is no file called "index.html".

shikhar-scs · 2018-01-02T13:31:48Z

@Rob--W
First of all, please ignore all .idea files as of now. I'll remove them in the next commit.
Also, the build failure has been because of indentation and stuff and I'll take care of it in my next commit for sure (gulp lint)

I've tried to incorporate as many changes as you had mentioned in your latest comment at #8632.
I've included the code for XMLHttp requests by taking help from other similar code written in the file. The code might be terribly wrong and you might have to help a bit more there.

Also, the request has been handled as mentioned by you in the test.js file and I've also tried writing those results using fs.writeFileSync . Again, as I had mentioned, the code I have written might be terribly wrong and you might need to help out there a bit.

The source folder has been giving an error on running gulp inject, I tried including build/generic/**/*.js but that didn't work too

Error: Missing positive glob
    at Object.create (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/glob-stream/index.js:73:39)
    at Gulp.src (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/gulp/node_modules/vinyl-fs/lib/src/index.js:33:23)
    at Gulp.<anonymous> (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/gulpfile.js:1181:15)
    at module.exports (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/orchestrator/lib/runTask.js:34:7)
    at Gulp.Orchestrator._runTask (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/orchestrator/index.js:273:3)
    at Gulp.Orchestrator._runStep (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/orchestrator/index.js:214:10)
    at Gulp.Orchestrator.start (/home/shikhar/Desktop/work/mozillaPdfJs/pdf.js/node_modules/orchestrator/index.js:134:8)
    at /usr/lib/node_modules/gulp-cli/lib/versioned/^3.7.0/index.js:54:20
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)

Please review.

shikhar-scs · 2018-01-04T01:00:44Z

@Rob--W mind reviewing ?

Rob--W · 2018-01-05T11:15:06Z

test/driver.js

+          var browserTestInfo = JSON.stringify(window.__coverage__);
+          re.send(browserTestInfo);
+        }
+      }


This code is not doing what you are intending. I suggest to read the tutorial and documentation at MDN to understand what the API is doing and how you should be using it instead:

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest

Rob--W · 2018-01-05T11:26:22Z

test/test.js

+  var parsedUrl = url.parse(req.url, true);
+  var pathname = parsedUrl.pathname;
+  if (pathname === '/browserTestReports') {
+    var information = req.body.browserTestInfo;


This req.body does not exist in the Node.js API.
To read the POST data (whatever you pass to the send method of XMLHttpRequest), you need to collect all data chunks and combine them. The "data" event will be emitted until there is no more data, and then the "end" event is emitted, upon which you can use all collected chunks and write it to a file. Instead of buffering all data, in this specific case you can also directly write the data to a file:

Use fs.createWriteStream to create a writable stream.

In the "data" event of req, call the write method on the writable stream.

In the "end" event of req, call the end method on the writable stream.

Here is an example from Node.js's core documentation: https://nodejs.org/api/stream.html#stream_writable_end_chunk_encoding_callback
Note that the chunks that you receive from the "data" event are Buffer objects instead of strings, but that is totally fine since the write method also accepts Buffers (as you can see from the documentation).

Rob--W · 2018-01-05T11:26:35Z

test/test.js

+  if (pathname === '/browserTestReports') {
+    var information = req.body.browserTestInfo;
+    var i = 0;
+    ++i;


What are you trying to do here?

I was trying to write the JSON recieved file by file at the location coverage/lcov-vreport/browserData/${i}.txt .

I had seen an example like the following one fs.writeFileSync(path.join(testSnapshotDir, (page + 1) + '.png'), testSnapshot); , but was confused on the file names so I derectly indexed them as of now.

You could obviously suggest some better methods.

Rob--W · 2018-01-05T11:27:51Z

package.json

    "gulp-rename": "^1.2.2",
    "gulp-replace": "^0.6.1",
    "gulp-transform": "^3.0.5",
-    "gulp-zip": "^4.1.0",
+    "gulp-util": "^3.0.8",


Where are you using this dependency?

I'll remove this ok

Rob--W · 2018-01-05T11:28:12Z

package.json

    "gulp-rename": "^1.2.2",
    "gulp-replace": "^0.6.1",
    "gulp-transform": "^3.0.5",
-    "gulp-zip": "^4.1.0",
+    "gulp-util": "^3.0.8",
+    "gulp-zip": "^4.0.0",


Why did you downgrade the gulp-zip dependency?

I'll change it back to the same

Rob--W · 2018-01-05T11:31:52Z

test/driver.js

@@ -582,6 +582,16 @@ var Driver = (function DriverClosure() { // eslint-disable-line no-unused-vars
      this._log('Done !');
      this.end.textContent = 'Tests finished. Close this window!';

+      var re = new XMLHttpRequest();
+      re.open('POST','/browserTestReports?path=' + escape(this.appPath),false);


This path query parameter is not used anywhere in test.js. Just use '/browserTestReports' . And add a space after the commas.

I'll make the changes

Rob--W · 2018-01-05T11:47:53Z

The source folder has been giving an error on running gulp inject, I tried including build/generic/**/*.js but that didn't work too
Error: Missing positive glob

I think that this error implies that no files are matched by your glob pattern. But this is not that important any more: you can completely remove the inject gulp task because the instrumented code should/will be loaded once you have applied the changes to test.js and test_slave.html in the way that I explained before.

Merge branch 'generate-test-coverage-statistics' of https://github.com/shikhar-scs/pdf.js into generate-test-coverage-statistics

shikhar-scs · 2018-01-25T12:40:23Z

@Rob--W

Rob--W · 2018-01-26T12:54:00Z

@shikhar-scs I've been preoccupied with other matters this week. I'll try to answer your question in the weekend. Thanks for your efforts and patience!

shikhar-scs · 2018-01-26T17:18:09Z

No problem @Rob--W,

Thanks for your efforts and patience!

I should be the one thanking you 😄

Rob--W · 2018-01-29T12:52:47Z

@shikhar-scs I have posted an answer at https://stackoverflow.com/a/48501707/938089 . It turns out that the tool can be used to convert multiple JSON files at once, so it is not necessary to try and merge all coverage files! That makes things much easier, e.g. you could write a coverage report for the main thread fo coverage-main.json and for the worker thread to coverage-worker.json.

shikhar-scs · 2018-01-29T18:54:23Z

Thannks a lot @Rob--W. I'll try and implement the same.

shikhar-scs · 2018-01-30T02:40:39Z

I tried generating the coverage reports using the istanbul reprt html command and it turns out to be better than our previous ones.

http://browsertestreports.bitballoon.com/ here is the link. A big improvement in the % since our last ones ( http://pdfjscoveragereport.bitballoon.com/ & http://coveragereportspdfjs.bitballoon.com/ )

It turns out that the tool can be used to convert multiple JSON files at once, so it is not necessary to try and merge all coverage files!

So shall I know remove all the changes pertaining to the merge we had made from this comment onwards #9308 (comment).

(If yes) Then I shall proceed with the same way for pdfjs-worker as I had initially with the normal pdf.js ? I guess something like this only

you could write a coverage report for the main thread fo coverage-main.json and for the worker thread to coverage-worker.json.

Rob--W · 2018-01-30T22:27:09Z

I tried generating the coverage reports using the istanbul reprt html command and it turns out to be better than our previous ones.

http://browsertestreports.bitballoon.com/ here is the link. A big improvement in the % since our last ones ( http://pdfjscoveragereport.bitballoon.com/ & http://coveragereportspdfjs.bitballoon.com/ )

This starts to look much, much better! 👍

It turns out that the tool can be used to convert multiple JSON files at once, so it is not necessary to try and merge all coverage files!

So shall I know remove all the changes pertaining to the merge we had made from this comment onwards #9308 (comment).

Not all of the changes. The part about merging can be removed (you can revert to using req.pipe to stream the JSON to a file). The part with ReportCoverageData needs to be kept.

(If yes) Then I shall proceed with the same way for pdfjs-worker as I had initially with the normal pdf.js ? I guess something like this only

you could write a coverage report for the main thread fo coverage-main.json and for the worker thread to coverage-worker.json.

Yes. You still need to figure out when the tests have completed and notify the worker of completion (.

shikhar-scs · 2018-01-31T12:50:03Z

Thank you @Rob--W for this. I'll follow all of it up soon 👍

shikhar-scs · 2018-01-31T14:19:37Z

@Rob--W I made the required changes back to facilitate pdf.worker.js . However, when I re ran the tests I got a warning that The API version "2.0.258" does not match the Worker version "2.0.270".
How do I set these to be the same ?

Rob--W · 2018-01-31T14:28:18Z

@shikhar-scs How are you running the tests? Can you push what you have so far for review?

shikhar-scs · 2018-01-31T14:44:20Z

@Rob--W done, please have a look 👍

Rob--W · 2018-01-31T15:56:45Z

gulpfile.js

@@ -1169,6 +1179,13 @@ gulp.task('lint', function (done) {
  });
 });

+gulp.task('instrument', function () {


Add , ['generic'] after 'instrument', to make sure that the generic target is run before instrument. Otherwise, if you run gulp instrument, you will add instrumentation to the existing build (which may not exist, or be outdated).

Rob--W · 2018-01-31T15:57:57Z

src/core/worker.js

+      re.setRequestHeader('Content-Type', 'application/json');
+      var coverageResults = JSON.stringify(self.__coverage__);
+      re.send(coverageResults);
+      handler.send('ReceivedCoverageData', null);


This line isn't doing anything and should be removed.

Rob--W · 2018-01-31T15:59:22Z

test/test.js

+function browserTestReportHandler(req, res) {
+  var parsedUrl = url.parse(req.url, true);
+  var pathname = parsedUrl.pathname;
+  if (pathname === '/browserTestReports' ||


There is no reason for having two API endpoints. Consider using /reportCoverageData as pathname, and in the code with new XMLHttpRequest(), call something like .open('POST', '/reportCoverageData?context=worker', false) (or context=main).

Then, the value that you passed above can be retrieved at the server with parsedUrl.query.context. Make sure that you validate that this parameter is a string (with typeof, opposed to being an array) and that the string matches the exact formats before using it (to avoid introducing a path traversal vulnerability).

After performing the appropriate validation, you can use fs.createWriteStream('../coverage/coverage-' + context + '.json');

Rob--W · 2018-01-31T16:09:06Z

test/test.js

+        workerWritableStream.on('finish', function () {
+          res.end();
+        });
+        req.on('ReceivedCoverageData', function () {


This event will never fire, so remove it. The type of req is http.IncomingMessage, and documented at https://nodejs.org/api/http.html#http_class_http_incomingmessage

shikhar-scs · 2018-02-01T02:27:45Z

@Rob--W I've made the required changes mentioned above apart from the incoming message strategy. Could you please clarify where do I have to use it exactly.

Rob--W · 2018-02-04T17:03:00Z

src/core/worker.js

@@ -355,6 +355,14 @@ var WorkerMessageHandler = {
    handler.on('GetDocRequest', function wphSetupDoc(data) {
      return WorkerMessageHandler.createDocumentHandler(data, port);
    });
+
+    handler.on('GetCoverageData', function workerCoverage() {


This should be reverted back to ReportCoverageData, since the handler handles the message that you send from here:
https://github.com/shikhar-scs/pdf.js/blob/beb7bd963db27adf6dac5d614002796c4a4de044/src/display/api.js#L1543-L1547

While you are at it - change workerCoverage to wphReportCoverageData for consistency with existing code.

Rob--W · 2018-02-04T17:06:59Z

test/test.js

+  var pathname = parsedUrl.pathname;
+  if (pathname === '/reportCoverageData') {
+    const pdfWritableStream = fs.createWriteStream(
+      '../coverage/coverage' + parsedUrl.query.context.toString() + '.json');


In my previous review, I wrote:

Then, the value that you passed above can be retrieved at the server with parsedUrl.query.context. Make sure that you validate that this parameter is a string (with typeof, opposed to being an array) and that the string matches the exact formats before using it (to avoid introducing a path traversal vulnerability).

After performing the appropriate validation, you can use fs.createWriteStream('../coverage/coverage-' + context + '.json');

The current change implements everything, except for the validation part. You should validate that parsedUrl.query.context has a safe value (e.g. by strictly checking whether parsedUrl.query.context is "worker" or "main" before using it. No need to use .toString() here if you validate in this way).

…m/shikhar-scs/pdf.js into generate-test-coverage-statistics

shikhar-scs · 2018-02-05T02:32:55Z

@Rob--W I've introduced the changes you had asked of in the last review. However now when I run the browserTests using

node test.js --browserManifestFile=resources/browser_manifests/browser_manifest.json --manifestFile=test_manifest.json --coverage

I get the following error

Loading file "pdfs/filled-background.pdf"
done (failed !: Loading PDF document: UnknownErrorException: The API version "2.0.258" does not match the Worker version "2.0.270".)

How can I make these versions similar ?

P.S. Please ignore the linting errors. Also, are you available on IRC or GITTER. I wanted to talk to you about something. 😄

Rob--W · 2018-02-05T10:51:17Z

Have you run gulp instrument to rebuild the instrumented files?

I'm on IRC, you can join the #pdfjs channel at irc.mozilla.org and ping me (robwu_nl).

shikhar-scs · 2018-02-05T11:25:51Z

Have you run gulp instrument to rebuild the instrumented files?

Yeah this corrected the errors. Thanks for this.

I ran my tests recently and my coveragemain.json file was created and I got all JSON data there. How do I trigger the pdf.worker.js related files so that I have a similar coverageworker.json file with data?

Also, please checkout your IRC

Rob--W · 2018-02-05T12:48:25Z

I ran my tests recently and my coveragemain.json file was created and I got all JSON data there. How do I trigger the pdf.worker.js related files so that I have a similar coverageworker.json file with data?

The ReportCoverageData message should have triggered a request. If the server is not receiving the message, try to investigate why that happens. Perhaps the message is not sent at all, e.g. because the WorkerTransport is never destroyed. Use the JavaScript debugger to set breakpoints and see what happens.

shikhar-scs · 2018-02-08T13:52:56Z

@Rob--W I tried running my tests, putting on stops and breakpoints (at all the writestream functions I've written and also at re.send at either of the places), taking help from (this link you had posted)[https://medium.com/@paul_irish/debugging-node-js-nightlies-with-chrome-devtools-7c4a1b95ae27]. However, surprisingly, none of the breakpoints were encountered 😢 .

Thus, as an alternative, I put console.log() statements at all the places where I had put the breakpoints. And this is what I got

i guess the Unknown action from worker: ReportCoverageData is the action that needs to be triggered. However, nothing of that sort is happening. Also each run takes about 15-20 minutes, so this consumes a lot of time too (I already ran it 4 times) . Is there a shorter way ?

A reminder, the problem that is occurring is that the coverageworker.json is not getting formed even though a coveragemain.json gets formed. Please could you look into it.

P.S. Is there any way to retrieve PM chats from irc ? (If no) are you available on slack ?

timvandermeij · 2019-01-20T13:45:29Z

Closing since this cannot be merged in the current state and isn't completely functional. However, the coverage reports provided here did help us a bit already in improving the unit test coverage, so that has certainly been useful. We'll keep the original issue open and refer to this pull request for inspiration if someone wants to continue working on this. Thanks.

Snuffleupagus reviewed Dec 22, 2017

View reviewed changes

Rob--W requested changes Dec 22, 2017

View reviewed changes

Rob--W mentioned this pull request Dec 22, 2017

Generate (test) coverage statistics #8632

Open

timvandermeij added the test label Dec 22, 2017

included jasmine with istanbul

ab239ce

shikhar-scs commented Dec 23, 2017

View reviewed changes

spec/helpers/jasmine_examples/SpecHelper.js Outdated

};

}

});

});

Copy link

Contributor Author

shikhar-scs Dec 23, 2017

Choose a reason for hiding this comment

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

ignore pls

shikhar-scs commented Dec 23, 2017

View reviewed changes

spec/jasmine_examples/PlayerSpec.js Outdated

}).toThrowError("song is already playing");

});

});

});

Copy link

Contributor Author

shikhar-scs Dec 23, 2017

Choose a reason for hiding this comment

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

ignore

shikhar-scs commented Dec 23, 2017

View reviewed changes

spec/support/jasmine.json Outdated

],

"stopSpecOnExpectationFailure": false,

"random": false

}

Copy link

Contributor Author

shikhar-scs Dec 23, 2017

Choose a reason for hiding this comment

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

ignore

removed lint errors

93eb09b

Snuffleupagus added the 4-work-in-progress label Dec 23, 2017

Rob--W requested changes Dec 24, 2017

View reviewed changes

shikhar-scs added 2 commits January 2, 2018 18:35

implementing changes

e662d41

Merge branch 'master' into generate-test-coverage-statistics

697d5d6

Rob--W requested changes Jan 5, 2018

View reviewed changes

shikhar-scs added 4 commits January 7, 2018 14:51

Removed the .idea folder

b62118b

changes in test, created readable stream and test query variable

23a8f3b

removed idea folder

88c6de3

Merge Conflicts

3c6b2e8

Merge branch 'generate-test-coverage-statistics' of https://github.com/shikhar-scs/pdf.js into generate-test-coverage-statistics

reverted changes

647ba0e

shikhar-scs force-pushed the generate-test-coverage-statistics branch from 8c89bb6 to 647ba0e Compare January 31, 2018 15:04

Rob--W reviewed Jan 31, 2018

View reviewed changes

new changes

beb7bd9

Rob--W requested changes Feb 4, 2018

View reviewed changes

shikhar-scs added 3 commits February 5, 2018 07:51

Merge branch 'master' into generate-test-coverage-statistics

12eda40

New Changes

6db3cee

Merge branch 'generate-test-coverage-statistics' of https://github.co…

88871b1

…m/shikhar-scs/pdf.js into generate-test-coverage-statistics

lint errors

da27bfd

timvandermeij closed this Jan 20, 2019

timvandermeij removed the 4-work-in-progress label Jan 20, 2019

jezhou mentioned this pull request Feb 13, 2020

Add code coverage to pdf.js #11580

Closed

		@@ -0,0 +1,2 @@
		repo_token: j0CpLqyVHdQZBFScn3qznXi3mY5uGuro4

enabled coverage reports for browsers Solves #8632 #9308

enabled coverage reports for browsers Solves #8632 #9308

Conversation

shikhar-scs commented Dec 22, 2017

Snuffleupagus left a comment • edited

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

shikhar-scs commented Dec 22, 2017 • edited

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

Rob--W commented Dec 22, 2017

Rob--W commented Dec 22, 2017

shikhar-scs commented Dec 22, 2017 • edited

shikhar-scs commented Dec 23, 2017 • edited

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

shikhar-scs commented Dec 23, 2017

Rob--W 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

shikhar-scs commented Jan 2, 2018 • edited

shikhar-scs commented Jan 4, 2018

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

Rob--W commented Jan 5, 2018

shikhar-scs commented Jan 25, 2018

Rob--W commented Jan 26, 2018

shikhar-scs commented Jan 26, 2018

Rob--W commented Jan 29, 2018

shikhar-scs commented Jan 29, 2018

shikhar-scs commented Jan 30, 2018

Rob--W commented Jan 30, 2018

shikhar-scs commented Jan 31, 2018

shikhar-scs commented Jan 31, 2018

Rob--W commented Jan 31, 2018

shikhar-scs commented Jan 31, 2018

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

shikhar-scs commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikhar-scs commented Feb 5, 2018 • edited

Rob--W commented Feb 5, 2018

shikhar-scs commented Feb 5, 2018 • edited

Rob--W commented Feb 5, 2018

shikhar-scs commented Feb 8, 2018

timvandermeij commented Jan 20, 2019

Snuffleupagus left a comment •

edited

shikhar-scs commented Dec 22, 2017 •

edited

shikhar-scs commented Dec 22, 2017 •

edited

shikhar-scs commented Dec 23, 2017 •

edited

shikhar-scs commented Jan 2, 2018 •

edited

shikhar-scs commented Feb 5, 2018 •

edited

shikhar-scs commented Feb 5, 2018 •

edited