1252995 - Add method names and uncovered lines to per-test coverage output

Status: RESOLVED FIXED

(Testing :: Code Coverage, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

In addition to covered lines, we'd like to be able to get method names and uncovered lines. Greg has a patch for this we'd like to get reviewed and landed.

Comment 1

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

This is the function that gathers all uncovered lines. It does so by going through each script and getting all lines that were not covered and have a count of 0. If a script has no coverage, we consider all lines in that script as uncovered. Then, we add into tempCovered all the lines which have been covered so that if we hit a second access to the script which has no coverage, we won't consider them as uncovered. Finally, we retrieve all uncovered lines which held a count of 0 until the end.

Review commit: https://reviewboard.mozilla.org/r/37873/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37873/

Comment 2

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add method names and uncovered lines to per-test coverage output.

This function is used to get the method names contained in the scripts. It returns an array containing keys in the form "lineNumber#methodName" that has each line number associated to a method. If the method is found to have an undefined name, we give it a name "undefined_integer" and every time we find a new undefined method, we increment the integer. There is the possibility that multiple functions can be caught on the same line. If a method has not been covered at all, we use lineNumber == '-1' to designate that it will not have any lines covered. This is needed to be able to have an empty covered array for the uncovered method.

Review commit: https://reviewboard.mozilla.org/r/37875/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37875/

Comment 3

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add method anmes and uncovered lines to per-test coverage.

This part is the modification to recordTestCoverage. In addition to what it did before, it now gets methods, their coverage, and uncovered lines of the entire script. The methods are obtained by going through the array returned from getMEthodNames, seperating the keys returned, and placing them into approriate records. The uncovered lines are retrieved in the same way that covered lines were retrieved.

Review commit: https://reviewboard.mozilla.org/r/37877/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37877/

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Thanks for the patches, Greg. I'll have some feedback here by end of day tomorrow.

Comment 5

[Trung Do]

Something that may be nice to have is a "version" attribute to describe the jscov format we're producing. This would make parsing the jscov files easier later on each time we have a format change.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review34883

What would you think of a much simpler approach, which would be to find every line with code on it from getAllOffsets on each script, and compare that to the lines we have as covered? I think that would achieve what we're aiming for here.

::: testing/modules/CoverageUtils.jsm:104
(Diff revision 1)
> +      

There are a couple of places mozreview is showing trailing white space, let's clean that up before checking this in. Many editors have a setting to show you this when you're typing, that can be quite helpful.

::: testing/modules/CoverageUtils.jsm:110
(Diff revision 1)
> +      cov = s.getAllColumnOffsets();

Why is getAllOffsets not sufficient for this? I don't see us making meaningful use of the column info.

::: testing/modules/CoverageUtils.jsm:113
(Diff revision 1)
> +        if(!currentUncovered[scriptName][lineNumber]){
> +          currentUncovered[scriptName][lineNumber] = 0;

I see properties on `currentUncovered` intialized to an empty set (above, on line 106), indexing like this makes me think they should be initialized to an empty object.

::: testing/modules/CoverageUtils.jsm:129
(Diff revision 1)
> +      if (!count){
> +        if (!currentUncovered[scriptName][lineNumber]){

In practice, do we reach this often? I'm curious whether no coverage is something getOffsetsCoverage is intended to capture.

::: testing/modules/CoverageUtils.jsm:152
(Diff revision 1)
> +  //Gather all lines uncovered based on whether they were counted or not.

Nit: I usually see a space after "//" in one-line comments.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726236 [details]
MozReview Request: Bug 1252995 - Add method names and uncovered lines to per-test coverage output.

https://reviewboard.mozilla.org/r/37875/#review34887

::: testing/modules/CoverageUtils.jsm:184
(Diff revision 1)
> +    let cov = s.getOffsetsCoverage();
> +    

Should we consider the job of this function to be to associate line numbers with method names, ignoring coverage? This might allow us to simplify things a bit, provided the output doesn't become too large. If this is an issue, we may want to do this once at the end of the test run and store it a seperate file.

::: testing/modules/CoverageUtils.jsm:207
(Diff revision 1)
> +      if (!this._allCoverage[scriptName]) {
> +        this._allCoverage[scriptName] = {};
> +      }

this._allCoverage isn't mentioned elsewhere in this function, this looks a bit out of place.

::: testing/modules/CoverageUtils.jsm:212
(Diff revision 1)
> +      if (!methodNames[scriptName][key]) {
> +        methodNames[scriptName][key] = key;

Can we model this as:

{
    <script name>: {
        <method name>: [ <lines> ]
        ...
    }
}

?

Having our "key" be both the key and value in this object is a little weird.

::: testing/modules/CoverageUtils.jsm:212
(Diff revision 1)
> +      if (!methodNames[scriptName][key]) {
> +        methodNames[scriptName][key] = key;

Can we model this as:

{
    <script name>: {
        <method name>: [ <lines> ]
        ...
    }
}

?

Having our "key" be both the key and value in this object is a little weird.

::: testing/modules/CoverageUtils.jsm:212
(Diff revision 1)
> +      if (!methodNames[scriptName][key]) {
> +        methodNames[scriptName][key] = key;

Can we model this as:

{
    <script name>: {
        <method name>: [ <lines> ]
        ...
    }
}

?

Having our "key" be both the key and value in this object is a little weird.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726237 [details]
MozReview Request: Bug 1252995 - Add method anmes and uncovered lines to per-test coverage.

https://reviewboard.mozilla.org/r/37877/#review34889

I think some developments in the earlier commits will impact what we have here, so I'll hold off reviewing for now.

Comment 9

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37873/#review34883

I think that this would be a great solution! I'm curious to see if this solution will catch more uncovered lines than what there is right now. I'll start implementing this.

Comment 10

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37873/#review34883

> In practice, do we reach this often? I'm curious whether no coverage is something getOffsetsCoverage is intended to capture.

Actually, it does return lines within a method or script which have not been counted. I am sure of that because the first few uncovered lines that I was catching were coming from here. However, I don't think this is useful anymore because I think they are now caught at lines 109-118 but I'll have to test that. If I change the way I get uncovered lines to the way you suggested above, I don't think this will be an issue anymore.

Comment 11

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37873/#review34883

> Why is getAllOffsets not sufficient for this? I don't see us making meaningful use of the column info.

With your proposed solution this shouldn't be an issue anymore. But, you are correct, getAllOffsets will be sufficient for this.

Comment 12

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37873/#review34883

> I see properties on `currentUncovered` intialized to an empty set (above, on line 106), indexing like this makes me think they should be initialized to an empty object.

I think this will be changed because of the propsed solution. But, if it doesn't I'll have it initialized to an empty object.

Comment 13

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37875/#review34887

> Should we consider the job of this function to be to associate line numbers with method names, ignoring coverage? This might allow us to simplify things a bit, provided the output doesn't become too large. If this is an issue, we may want to do this once at the end of the test run and store it a seperate file.

We definitely can and it would be much simpler. I think that the size will be increased by quite a bit, but I'd have to see by how much. It will go up by quite a bit though because there would be a lot of duplicate values in the methods and uncovered arrays. I'll test out this solution first and let you now how big the file gets.

> Can we model this as:
> 
> {
>     <script name>: {
>         <method name>: [ <lines> ]
>         ...
>     }
> }
> 
> ?
> 
> Having our "key" be both the key and value in this object is a little weird.

I'll have it set up this way for the next review.

Comment 14

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726237 [details]
MozReview Request: Bug 1252995 - Add method anmes and uncovered lines to per-test coverage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37877/diff/1-2/

Comment 15

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Uncovered Lines revision.

This method now compares all offsets to the lines covered to determine uncovered lines.

Review commit: https://reviewboard.mozilla.org/r/38827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38827/

Comment 16

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - getMethodNames revision.

The methods are now obtained along with the lines that they contain, regardless of coverage.

Review commit: https://reviewboard.mozilla.org/r/38829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38829/

Comment 17

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - recordTestCoverage revision.

It's now much simpler than what it was before. Methods, uncovered lines, and covered lines are all pushed to the record in a similar way

Review commit: https://reviewboard.mozilla.org/r/38853/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38853/

Comment 18

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Whitespace removal

Review commit: https://reviewboard.mozilla.org/r/38855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38855/

Comment 19

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37875/#review34887

> We definitely can and it would be much simpler. I think that the size will be increased by quite a bit, but I'd have to see by how much. It will go up by quite a bit though because there would be a lot of duplicate values in the methods and uncovered arrays. I'll test out this solution first and let you now how big the file gets.

The JSON output is now quite large ~1,800kB for the single JSON from running browser/components/newtab/tests/browser/.

Comment 20

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/1-2/

Comment 21

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add method names and uncovered lines to per-test coverage output.

This function is used to get the method names contained in the scripts. It returns an array containing keys in the form "lineNumber#methodName" that has each line number associated to a method. If the method is found to have an undefined name, we give it a name "undefined_integer" and every time we find a new undefined method, we increment the integer. There is the possibility that multiple functions can be caught on the same line. If a method has not been covered at all, we use lineNumber == '-1' to designate that it will not have any lines covered. This is needed to be able to have an empty covered array for the uncovered method.

* * *
Bug 1252995 - Method names revision.
This is the revision for getMethodNames. It now uses getAllOffsets to get the lines each method covers.

Review commit: https://reviewboard.mozilla.org/r/39361/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39361/

Comment 22

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add method anmes and uncovered lines to per-test coverage.

* * *
Bug 1252995 - recordTestCoverage revision.
This is a modification to recordTestCoverage. It now get methods and all lines they cover, has a version control block, and gets uncovered lines also.

Review commit: https://reviewboard.mozilla.org/r/39363/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39363/

Comment 23

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729639 [details]
MozReview Request: Bug 1252995 - Add method names and uncovered lines to per-test coverage output.

This function is used to get the method names contained in the scripts. It returns an array containing keys in the form "lineNumber#methodName" that has each line number associated to a method. If the method is found to have an undefined name, we give it a name "undefined_integer" and every time we find a new undefined method, we increment the integer. There is the possibility that multiple functions can be caught on the same line. If a method has not been covered at all, we use lineNumber == '-1' to designate that it will not have any lines covered. This is needed to be able to have an empty covered array for the uncovered method.

* * *
Bug 1252995 - Method names revision.
This is the revision for getMethodNames. It now uses getAllOffsets to get the lines each method covers.

Review commit: https://reviewboard.mozilla.org/r/39361/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39361/

Comment 24

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729640 [details]
MozReview Request: Bug 1252995 - Add method anmes and uncovered lines to per-test coverage.

* * *
Bug 1252995 - recordTestCoverage revision.
This is a modification to recordTestCoverage. It now get methods and all lines they cover, has a version control block, and gets uncovered lines also.

Review commit: https://reviewboard.mozilla.org/r/39363/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39363/

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review36237

Comment 26

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/2-3/

Comment 27

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

This method gets method names and the lines each method spans. It uses getAllOffsets to get the lines.

Review commit: https://reviewboard.mozilla.org/r/39569/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39569/

Comment 28

[Greg Mierzwinski [:sparky]]

Attachment: MozReview Request: Bug 1252995 - recordTestCoverage modification.

This is a modification to recordTestCoverage. It now gathers methods and the lines each span, uncovered lines, and now also places a version control block at the beginning of every artifact.

Review commit: https://reviewboard.mozilla.org/r/39571/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39571/

Comment 29

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review36427

Looking good, a few small things we should address before landing.

::: testing/modules/CoverageUtils.jsm:101
(Diff revision 3)
> +CoverageCollector.prototype._getUncoveredLines = function() {
> +  let uncoveredLines = {};
> +  let tempUncov = {};
> +  this._scripts.forEach(s => {
> +    let scriptName = s.url;
> +    let cov2 = s.getAllOffsets();

Let's name this variable something else, like "scriptOffsets", it's not recording coverage here.

::: testing/modules/CoverageUtils.jsm:104
(Diff revision 3)
> +    if(!tempUncov[scriptName]){
> +        tempUncov[scriptName] = cov2;

It looks like this needs to be initialized to an empty object.

::: testing/modules/CoverageUtils.jsm:109
(Diff revision 3)
> +    if(!tempUncov[scriptName]){
> +        tempUncov[scriptName] = cov2;
> +    }
> +
> +    // Get all lines in the script
> +    cov2.forEach( function(element, index, array) {

Unused `array` parameter, no need to specify it in this signature.

::: testing/modules/CoverageUtils.jsm:121
(Diff revision 3)
> +    });
> +  });
> +
> +  // For all covered lines, set their value to -1
> +  for (let scriptName in this._allCoverage){
> +    for(let key in this._allCoverage[scriptName]){

Not: space after `for`.

::: testing/modules/CoverageUtils.jsm:123
(Diff revision 3)
> +
> +  // For all covered lines, set their value to -1
> +  for (let scriptName in this._allCoverage){
> +    for(let key in this._allCoverage[scriptName]){
> +      let [lineNumber, columnNumber, offset] = key.split('#');
> +      tempUncov[scriptName][lineNumber] = -1;

Alternatively, remove entries here (this would simplify the loop below as well).

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

https://reviewboard.mozilla.org/r/39569/#review36431

Very nice. I'd just like to take another look after we resolve the question below.

::: testing/modules/CoverageUtils.jsm:156
(Diff revision 1)
> +    // Give method a default name if it does not exist
> +    if (!method){
> +      method = "unknown_" + temp++;
> +    }

Can we just return early if we have an unnamed method? Are these going to end up being useful in the output?

::: testing/modules/CoverageUtils.jsm:167
(Diff revision 1)
> +    * Get all lines contained within the method and
> +    * push a record of the form:
> +    * <method name> : <lines covered>
> +    */
> +    covTemp.forEach(function (element, index, array){
> +      if(!element){

Nit: space after `if`.

Comment 31

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

https://reviewboard.mozilla.org/r/39571/#review36435

This is close. Can we see the result of this modification on try before the next round of review? Thanks.

::: testing/modules/CoverageUtils.jsm:186
(Diff revision 1)
> +  let methods = this._getMethodNames(testName);
> +  let uncoveredLines = this._getUncoveredLines(testName);

Neither of these methods take an argument, you can leave `testName` out here.

::: testing/modules/CoverageUtils.jsm:196
(Diff revision 1)
> +
>    for (let scriptName in rawLines) {
>      let rec = {
>        testUrl: testName,
>        sourceFile: scriptName,
> -      covered: []
> +      method: [],

Let's call this "methods" and re-name the variable above, and let's make it an object, not an array.

::: testing/modules/CoverageUtils.jsm:203
(Diff revision 1)
> +        let methodRec = {method: methodLines, cov: covering};
> +        rec.method.push(methodRec)

If we're able to make this an object, not an array, we can use the method name as a key, and the array of covered lines as a value. How does that sound?

Comment 32

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/39569/#review36431

> Can we just return early if we have an unnamed method? Are these going to end up being useful in the output?

We can return early here but if we do then we miss the first script which seems to almost always be the globals of the script. I say almost always because I don't remember any source file that doesn't have it. I'm not sure if that's useful information though. The other unnamed methods seem to be a limitation of the Debugger, so maybe if the limitations are overcome in the future they could become as useful as the named ones. Should I take it out for now?

Comment 33

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/3-4/

Comment 34

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39569/diff/1-2/

Comment 35

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39571/diff/1-2/

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review39345

This looks good. I'm going to hold off on r+ until we get this tested on try and we can sanity check this all fits together as expected. I have a few more comments, which you may want to address if you feel inclined, but are strictly optional for this bug.

::: testing/modules/CoverageUtils.jsm:104
(Diff revision 4)
> +  this._scripts.forEach(s => {
> +    let scriptName = s.url;
> +    let scriptOffsets = s.getAllOffsets();
> +
> +    if (!tempUncov[scriptName]){
> +        tempUncov[scriptName] = {};

This is fine, but a Set might be a more appropriate data structure now that I think about it. I should have noticed this before. Convert before landing if you think that's a worthwhile change.

::: testing/modules/CoverageUtils.jsm:122
(Diff revision 4)
> +
> +  // For all covered lines, set their value to 0
> +  for (let scriptName in this._allCoverage){
> +    for (let key in this._allCoverage[scriptName]){
> +      let [lineNumber, columnNumber, offset] = key.split('#');
> +      tempUncov[scriptName][lineNumber] = 0;

Alternatively, use the `delete` operator here.

::: testing/modules/CoverageUtils.jsm:133
(Diff revision 4)
> +    for (let line in tempUncov[scriptName]){
> +      if (!uncoveredLines[scriptName]){
> +        uncoveredLines[scriptName] = new Set();
> +      }
> +      if (tempUncov[scriptName][line]){
> +        uncoveredLines[scriptName].add(parseInt(line,10));

Nit: space after `,`

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

https://reviewboard.mozilla.org/r/39569/#review39349

Looking good. Just a few nits that should be addressed.

::: testing/modules/CoverageUtils.jsm:152
(Diff revision 2)
> +    let scriptOffsets = s.getAllOffsets();
> +
> +    if (!methodNames[scriptName]){
> +      methodNames[scriptName] = {};
> +    }
> +    // Give method a default name if it does not exist

This comment should be updated now.

::: testing/modules/CoverageUtils.jsm:153
(Diff revision 2)
> +    if (!method){
> +      return;
> +    }

This early return could be moved closer to the declaration of `method`. I think this would make the function a little easier to read.

::: testing/modules/CoverageUtils.jsm:162
(Diff revision 2)
> +    scriptOffsets.forEach(function (element, index){
> +      if (!element){
> +        return;
> +      }
> +      tempMethodCov.push(index);
> +    });

To save space, we may be able to just store [start, end] instead of every line, but let's see how this goes, we may not need to.

Comment 38

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

https://reviewboard.mozilla.org/r/39571/#review39357

This is pretty much ready to land, I'll go back over this once we've seen it on try and you've updated any issues. Thanks!

::: testing/modules/CoverageUtils.jsm:197
(Diff revision 2)
> +    for (let methodLines in methods[scriptName]){
> +      rec.methods[methodLines] = methods[scriptName][methodLines];

Is `methodLines` a good name for this variable? It's more like `methodName`, right?

Comment 39

[Greg Mierzwinski [:sparky]]

https://reviewboard.mozilla.org/r/37873/#review39345

> Alternatively, use the `delete` operator here.

I tried using the delete operator but it isn't deleting anything for some odd reason. I've got something else I'm going to try to get it working but if it doesn't work then I'll just leave it as it is.

Comment 40

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/4-5/

Comment 41

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39569/diff/2-3/

Comment 42

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39571/diff/2-3/

Comment 43

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review40385

::: testing/modules/CoverageUtils.jsm:104
(Diff revisions 4 - 5)
>    this._scripts.forEach(s => {
>      let scriptName = s.url;
>      let scriptOffsets = s.getAllOffsets();
>  
>      if (!tempUncov[scriptName]){
> -        tempUncov[scriptName] = {};
> +        tempUncov[scriptName] = new Set();

If we move to a Set here, we should also modify interactions with the data structure, so below we'd have `tempUncov[scriptName].add(index)`, etc.

Comment 44

[Chris Manchester (limited bugmail, email directly)]

I have this on try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=232db2940e992fe4c998dbb0a498703fbf463e0d

Assuming I applied everything correctly, we should have coverage json to look at in a few hours.

Comment 45

[Trung Do]

As I'm seeing it the "version" property is currently inside an object (the first one) inside the outermost array. This requires the parser to know that the first object inside the array is not like the others, which I think is not optimal. A better way to organize it is to do something like

{
    "meta": {
        "version": 1
    },
    "data": [
        // the original array of jscov objects 
    ]
}

Comment 46

[Trung Do]

Also, I think this format is nicer for the "methods" property:

    "methods": [
        {
            "name": "XPCU_generateQI",
            "covered": [
                110,
                ...
            ]
        },
        {
            "name": "XPCU_generateCI",
            "covered": [
                132,
                ...
            ],
       },
       ...
    ]

It is more consistent with the existing object structure.

Comment 47

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

https://reviewboard.mozilla.org/r/39569/#review41153

Comment 48

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

https://reviewboard.mozilla.org/r/39571/#review41155

I think Trung's suggestion from comment 45 makes a lot of sense, but no need to block landing this!

Comment 49

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/5-6/

Comment 50

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39569/diff/3-4/

Comment 51

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39571/diff/3-4/

Comment 52

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review41613

::: testing/modules/CoverageUtils.jsm
(Diff revisions 5 - 6)
> -    for (let line in tempUncov[scriptName]){
> -      if (!uncoveredLines[scriptName]){
> +    if (!uncoveredLines[scriptName]){
> -        uncoveredLines[scriptName] = new Set();
> +      uncoveredLines[scriptName] = new Set();
> -      }
> +    }
> -      if (tempUncov[scriptName][line]){
> -        uncoveredLines[scriptName].add(parseInt(line, 10));
> +    for (let line of tempUncov[scriptName]){
> +      uncoveredLines[scriptName].add(line);
> -      }

I was going to suggest just doing `uncoveredLines[scriptName] = tempUncov[scriptName];`, but looking at this again, I think we can just get rid of `tempUncov` altogether and modify `uncoveredLines` througout.

Comment 53

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/6-7/

Comment 54

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39569/diff/4-5/

Comment 55

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39571/diff/4-5/

Comment 56

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

https://reviewboard.mozilla.org/r/37873/#review42041

Looking good. Just one small nit that should be fixed before landing. Thanks!

::: testing/modules/CoverageUtils.jsm:111
(Diff revisions 6 - 7)
> -      if (!tempUncov[scriptName].index){
> -        tempUncov[scriptName].add(index);
> +      if (!uncoveredLines[scriptName].index){
> +        uncoveredLines[scriptName].add(index);
>        }

Nit: No need to guard here, the set will not retain duplicates.

Comment 57

[Greg Mierzwinski [:sparky]]

Comment on attachment 8726235 [details]
MozReview Request: Bug 1252995 - Add a method to get uncovered lines.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37873/diff/7-8/

Comment 58

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729738 [details]
MozReview Request: Bug 1252995 - Add a method to get lines and name of methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39569/diff/5-6/

Comment 59

[Greg Mierzwinski [:sparky]]

Comment on attachment 8729739 [details]
MozReview Request: Bug 1252995 - recordTestCoverage modification.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39571/diff/5-6/

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b72f1f574aba
https://hg.mozilla.org/integration/mozilla-inbound/rev/1602320ae208
https://hg.mozilla.org/integration/mozilla-inbound/rev/c155349cb403

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b72f1f574aba
https://hg.mozilla.org/mozilla-central/rev/1602320ae208
https://hg.mozilla.org/mozilla-central/rev/c155349cb403