1317019 - Fix JS Debugger code coverage collection for when source file has no methods covered.

Status: RESOLVED FIXED

(Testing :: Code Coverage, defect)

Description

[Greg Mierzwinski [:sparky]]

This bug occurs in "CoverageUtils.jsm" and because there is no condition that checks if a given source file has methods that were touched/used. "test_PrivateTemp.js" is one of the tests that cause this problem to occur.

Comment 1

[Greg Mierzwinski [:sparky]]

Attachment: Bug 1317019 - Fix object iteration and check if scriptName exists.

This patch will prevent method-level code coverage recording when a source file does not contain any methods in the 'methods' set.

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

Comment 2

[Greg Mierzwinski [:sparky]]

Here's an example for before and after applying the patch: https://pastebin.mozilla.org/8927404 .

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8810021 [details]
Bug 1317019 - Fix object iteration and check if scriptName exists.

https://reviewboard.mozilla.org/r/92486/#review92850

Comment 4

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ee4b7c1cac

Comment 5

[Greg Mierzwinski [:sparky]]

Comment on attachment 8810021 [details]
Bug 1317019 - Fix object iteration and check if scriptName exists.

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

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8810021 [details]
Bug 1317019 - Fix object iteration and check if scriptName exists.

https://reviewboard.mozilla.org/r/92486/#review110552

::: testing/modules/CoverageUtils.jsm:169
(Diff revision 2)
> + */
> +Object.prototype[Symbol.iterator] = function * () {
> +  for (var [key, value] of Object.entries(this)) {
> +    yield [key, value];
> +  }
> +};

do we have a danger of accidentally using this in the future while adding code to this module?  Not too concerning but I would like your thoughts on this

Comment 7

[Greg Mierzwinski [:sparky]]

Comment on attachment 8810021 [details]
Bug 1317019 - Fix object iteration and check if scriptName exists.

https://reviewboard.mozilla.org/r/92486/#review110552

> do we have a danger of accidentally using this in the future while adding code to this module?  Not too concerning but I would like your thoughts on this

I was wondering the same thing and I think that it could be accidently used, but that usage wouldn't be problematic.

This simply implements an iterator for iterating over objects that are within objects (by objects, I mean variables created using '{}') so it shouldn't affect anything outside of this. I think that at worst, if it is used accidently, it would help with iterating over an object within another object. So, the problem we encounter seems like more of a bonus for me - especially since it makes the code more clear, & elegant.

In my opinon, even if this module is included in another file and used completely, it'll do the same thing that I mention above. So, I'm actually having a hard time trying to think of where this could actually be problematic and break something. Which, I believe, is a good thing. :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddd06f373719
Fix object iteration and check if scriptName exists. r=chmanchester,jmaher

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ddd06f373719