-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
enabled coverage reports for browsers Solves #8632 #9308
enabled coverage reports for browsers Solves #8632 #9308
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
.gitignore
Outdated
@@ -8,3 +8,4 @@ node_modules/ | |||
examples/node/svgdump/ | |||
examples/node/pdf2png/*.png | |||
package-lock.json | |||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be here, please remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will do that in my next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is still here (don't forget to remove the new trailing line too).
No problem @Snuffleupagus Ill wait for @Rob--W
Yup sure I'll do that . That actually is a result of squashing |
.coveralls.yml
Outdated
@@ -0,0 +1,2 @@ | |||
repo_token: j0CpLqyVHdQZBFScn3qznXi3mY5uGuro4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W I'll keep that in mind.
package.json
Outdated
"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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused dependency - remove this.
package.json
Outdated
} | ||
] | ||
] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused code - remove the whole browserify
block.
package.json
Outdated
"scripts": { | ||
"test": "gulp lint unittestcli externaltest" | ||
"test": "gulp lint unittestcli externaltest", | ||
"cover": "cd build && cd lib && istanbul cover --include-all-sources jasmine-node test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know that. I'll try and use jasmine here instead.
package.json
Outdated
"babel-preset-stage-0": "^6.24.1", | ||
"babel-register": "^6.26.0", | ||
"jasmine-node": "^1.14.5" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these dependencies are used - remove them. In the future, if you add new dependencies, store them in the devDependencies
dictionary.
.babelrc
Outdated
"plugins": [ "istanbul" ] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file used? I don't think so, so let's remove it.
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
After running the above steps, we get test coverage for There are two concerns in this set up:
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. |
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 Lines 1129 to 1140 in e081a70
Note that the So to get the same logic as in
|
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. |
@Rob--W
1 and 2 was manged from the blog mentioned above, however, now, when I run
This token as you might guess is Also, I've tried generating unittestcli reports as suggested in the comment at #8632 using the statement Another good news is that now we can run tests directly from pdf.js folder using these commands
And indeed test reports now get generated from here itself 🎆 (which previously was not happening and thus we had to move into build > lib) There have been significant improvements in the percentages, however, again ES5 and ES6 problems have persisted till now and thus
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. |
lib/jasmine_examples/Player.js
Outdated
this.currentlyPlayingSong.persistFavoriteStatus(true); | ||
}; | ||
|
||
module.exports = Player; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore this file. I'll delete it in the next commit.
lib/jasmine_examples/Song.js
Outdated
throw new Error("not yet implemented"); | ||
}; | ||
|
||
module.exports = Song; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore this file too
}; | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore pls
spec/jasmine_examples/PlayerSpec.js
Outdated
}).toThrowError("song is already playing"); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore
spec/support/jasmine.json
Outdated
], | ||
"stopSpecOnExpectationFailure": false, | ||
"random": false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore
primarily, changes have been made in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
.gitignore
Outdated
@@ -8,3 +8,4 @@ node_modules/ | |||
examples/node/svgdump/ | |||
examples/node/pdf2png/*.png | |||
package-lock.json | |||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is still here (don't forget to remove the new trailing line too).
gulpfile.js
Outdated
@@ -84,6 +86,11 @@ var DEFINES = { | |||
SKIP_BABEL: false, | |||
}; | |||
|
|||
var ISTANBUL_PATHS = { | |||
'javascript': ['coverage/**/*.js'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the single quotes around 'javascript'
.
gulpfile.js
Outdated
@@ -1168,6 +1175,29 @@ gulp.task('lint', function (done) { | |||
}); | |||
}); | |||
|
|||
gulp.task('instrument', function () { | |||
return gulp.src(['src/**/*.js']) | |||
// Covering files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
package.json
Outdated
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "git://github.com/mozilla/pdf.js.git" | ||
}, | ||
"license": "Apache-2.0" | ||
"license": "Apache-2.0", | ||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the dependencies below to devDependencies
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rob--W Thanx for all these reviews, I'll make all the changes accordingly.
gulpfile.js
Outdated
return gulp.src(['src/**/*.js']) | ||
// Covering files | ||
.pipe(istanbul({ coverageVariable: '__coverage__', | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting looks a bit weird. Can you merge it with the previous line?
gulpfile.js
Outdated
@@ -1168,6 +1175,29 @@ gulp.task('lint', function (done) { | |||
}); | |||
}); | |||
|
|||
gulp.task('instrument', function () { | |||
return gulp.src(['src/**/*.js']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using ISTANBUL_PATHS
elsewhere and an array here?
gulpfile.js
Outdated
.pipe(gulp.dest('coverage/')); | ||
}); | ||
|
||
gulp.task('inject', ['instrument'], function (cb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename cb
to done
and actually call the callback when the task completes.
gulpfile.js
Outdated
}); | ||
|
||
gulp.task('inject', ['instrument'], function (cb) { | ||
return gulp.src('index.html') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole task is not doing anything because there is no file called "index.html".
@Rob--W I've tried to incorporate as many changes as you had mentioned in your latest comment at #8632. Also, the request has been handled as mentioned by you in the The source folder has been giving an error on running gulp inject, I tried including
Please review. |
@Rob--W mind reviewing ? |
test/driver.js
Outdated
var browserTestInfo = JSON.stringify(window.__coverage__); | ||
re.send(browserTestInfo); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on it 👍
test/test.js
Outdated
var parsedUrl = url.parse(req.url, true); | ||
var pathname = parsedUrl.pathname; | ||
if (pathname === '/browserTestReports') { | ||
var information = req.body.browserTestInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thewrite
method on the writable stream. - In the "end" event of
req
, call theend
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).
test/test.js
Outdated
if (pathname === '/browserTestReports') { | ||
var information = req.body.browserTestInfo; | ||
var i = 0; | ||
++i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you trying to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
package.json
Outdated
"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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you using this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this ok
package.json
Outdated
"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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you downgrade the gulp-zip dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it back to the same
test/driver.js
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path
query parameter is not used anywhere in test.js. Just use '/browserTestReports'
. And add a space after the commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make the changes
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 |
Merge branch 'generate-test-coverage-statistics' of https://github.com/shikhar-scs/pdf.js into generate-test-coverage-statistics
@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! |
No problem @Rob--W,
I should be the one thanking you 😄 |
@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 |
Thannks a lot @Rob--W. I'll try and implement the same. |
I tried generating the coverage reports using the 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/ )
So shall I know remove all the changes pertaining to the (If yes) Then I shall proceed with the same way for
|
This starts to look much, much better! 👍
Not all of the changes. The part about merging can be removed (you can revert to using
Yes. You still need to figure out when the tests have completed and notify the worker of completion (. |
Thank you @Rob--W for this. I'll follow all of it up soon 👍 |
@Rob--W I made the required changes back to facilitate |
@shikhar-scs How are you running the tests? Can you push what you have so far for review? |
@Rob--W done, please have a look 👍 |
8c89bb6
to
647ba0e
Compare
gulpfile.js
Outdated
@@ -1169,6 +1179,13 @@ gulp.task('lint', function (done) { | |||
}); | |||
}); | |||
|
|||
gulp.task('instrument', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/core/worker.js
Outdated
re.setRequestHeader('Content-Type', 'application/json'); | ||
var coverageResults = JSON.stringify(self.__coverage__); | ||
re.send(coverageResults); | ||
handler.send('ReceivedCoverageData', null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line isn't doing anything and should be removed.
test/test.js
Outdated
function browserTestReportHandler(req, res) { | ||
var parsedUrl = url.parse(req.url, true); | ||
var pathname = parsedUrl.pathname; | ||
if (pathname === '/browserTestReports' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
test/test.js
Outdated
workerWritableStream.on('finish', function () { | ||
res.end(); | ||
}); | ||
req.on('ReceivedCoverageData', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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. |
src/core/worker.js
Outdated
@@ -355,6 +355,14 @@ var WorkerMessageHandler = { | |||
handler.on('GetDocRequest', function wphSetupDoc(data) { | |||
return WorkerMessageHandler.createDocumentHandler(data, port); | |||
}); | |||
|
|||
handler.on('GetCoverageData', function workerCoverage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/test.js
Outdated
var pathname = parsedUrl.pathname; | ||
if (pathname === '/reportCoverageData') { | ||
const pdfWritableStream = fs.createWriteStream( | ||
'../coverage/coverage' + parsedUrl.query.context.toString() + '.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (withtypeof
, 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
@Rob--W I've introduced the changes you had asked of in the last review. However now when I run the browserTests using
I get the following error
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. 😄 |
Have you run I'm on IRC, you can join the |
Yeah this corrected the errors. Thanks for this. I ran my tests recently and my Also, please checkout your IRC |
The |
@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 i guess the A reminder, the problem that is occurring is that the P.S. Is there any way to retrieve PM chats from |
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. |
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 buildAlso 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