Bug 117941 – code coverage report

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alex Christensen]

The script generate-coverage-data has no way to view its results.

[Alex Christensen]

Created attachment 205325 [details]
Patch

[Alex Christensen]

Created attachment 205327 [details]
Patch

[Build Bot]

Comment on attachment 205327 [details]
Patch

Attachment 205327 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/953201

New failing tests:
fast/media/mq-transform-03.html
fast/forms/search-event-delay.html
fast/media/mq-transform-02.html
platform/win/accessibility/multiple-select-element-role.html

[Build Bot]

Created attachment 205350 [details]
Archive of layout-test-results from APPLE-EWS-3 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-3  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Alex Christensen]

There is no way this caused any failures because the only changes were to tools that nobody uses right now.

[Joseph Pecoraro]

Comment on attachment 205327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205327&action=review

Overall this looks good. I'd like to see a second patch with more style consistency and a few of the other comments addressed, but the logic looks good so r+

> Tools/CodeCoverage/report.html:77
> +                if(hits == -1)
> +                    return 'white'; // Non-code lines
> +                else if(hits == 0)
> +                    return 'orange'; // Unexecuted lines

Style: "if(" => "if ("

Suggestion: For JS in the Web Inspector we prefer to use === wherever possible. These and others could be made strict equality.

> Tools/CodeCoverage/report.html:79
> +                    // Executed lines are between red and green

Nit: Comments should be sentences and end with a period. Here and throughout the patch.

> Tools/CodeCoverage/report.html:81
> +                    return 'rgb('+ relativeHeat + ',' + (255 - relativeHeat) + ', 0)';

Style: space between substrings, so between 'rob(' and the +.

> Tools/CodeCoverage/report.html:88
> +                return 'rgb('+ (255 - value) + ',' + value + ', 0)';

Ditto.

> Tools/CodeCoverage/report.html:93
> +                var children = this.parentNode.childNodes[this.parentNode.childNodes.length - 1];

Suggestion: this.parentNode.lastChild

> Tools/CodeCoverage/report.html:114
> +                for (var i = 0; i < fileData.hitLines.length; i++)

Suggestion: Newlines before/after these for loops would make the code a little more readable.

> Tools/CodeCoverage/report.html:133
> +                    textColumn.style.background = getHeatBackgroundColor(hits[i],fileData.maxHeat);

Style: space after comma

> Tools/CodeCoverage/report.html:135
> +                    textColumn.style.whiteSpace = 'nowrap';
> +                    textColumn.style.fontFamily = 'Courier, monospace';

How about giving the textColumn a className and putting these styles in CSS?

> Tools/CodeCoverage/report.html:146
> +            function fileClicked()

I'd prefer to see an "event" parameter to this click handler instead of depending on window.event below. Being explicit makes code easier to follow.

> Tools/CodeCoverage/report.html:154
> +                var xmlhttp=new XMLHttpRequest();
> +                xmlhttp.onreadystatechange=function()
> +                {
> +                    if (xmlhttp.readyState==4) {
> +                        var codeviewer = document.getElementById('codeviewer');
> +                        codeviewer.replaceChild(processFile(xmlhttp.fileData,xmlhttp.responseText), codeviewer.childNodes[0]);
> +                    }

Style: spaces around =, ==, and comma. This comment applies here and throughout the code.

"xmlhttp" is a weird name, how about "xhr"?

> Tools/CodeCoverage/report.html:190
> +                a.href = 'javascript:void(0)';

We try to avoid javascript: whenever possible. This could just be:

    a.href = '#';

And inside the event handler (fileClicked) you can call:

    event.preventDefault();

> Tools/CodeCoverage/report.html:191
> +                a.onclick = fileClicked;

I'd prefer to see addEventListener. How about:

    a.addEventListener('click', fileClicked.bind(a));

Or just "fileClicked" and instead of using "this" in the handler you could use "event.target".

> Tools/CodeCoverage/report.html:211
> +                var subdirNames = [];
> +                var fileNames = [];
> +                for (var subdir in dir.subdirs)
> +                    subdirNames.push(subdir);
> +                for (var file in dir.files)
> +                    fileNames.push(file);
> +                subdirNames.sort();
> +                fileNames.sort();

Seems like this could be:

    var subdirNames = Object.keys(dir.subdirs).sort();
    var fileNames = Object.keys(dir.files).sort();

> Tools/CodeCoverage/report.html:222
> +                img.onclick = expandClicked;

Style: addEventListener. You may need to bind so that "this" makes sense in the handler.

> Tools/CodeCoverage/report.html:235
> +                li.appendChild(makeGraphs(dir));

There is a lot of makeGraphs going on. Could some of this be done lazily like when you click an item?

> Tools/CodeCoverage/report.html:291
> +                directory.coverage = directory.totalHitLines / directory.totalLines;

Is it possible that directory.totalLines is 0? If so then coverage would be Infinity. You should 0 check like you do with branchCoverage just in case.

> Tools/CodeCoverage/report.html:314
> +                var directory = {};

How about naming this rootDirectory, or something more descriptive?

> Tools/CodeCoverage/report.html:336
> +                directories.replaceChild(report, directories.childNodes[0]);

Suggestion: directories.childNodes[0] => directories.firstChild

> Tools/Scripts/generate-coverage-data:80
> +# Open report
> +my $url = 'file://' . sourceDir() . '/Tools/CodeCoverage/report.html?' . $resultName;
> +system "open -g -a Safari";
> +system "osascript -e \"tell application \\\"Safari\\\" to make new document with properties {URL:\\\"" . $url . "\\\"} at front\"";

Why not just do:

    system "open '$url'";

Does the Apple Script (Mac only by the way) do anything special?

It might also be nice to have a way to open a URL that supports other OSs, like linux. I see there are some Perl modules that do that, but nothing built-in.

[Alex Christensen]

> Why not just do:
> 
>     system "open '$url'";
> 
> Does the Apple Script (Mac only by the way) do anything special?
> 
> It might also be nice to have a way to open a URL that supports other OSs, like linux. I see there are some Perl modules that do that, but nothing built-in.

I also want a cross-platform way to open a url, but system "open '$url'" removes the query, which I use to locate the data file.

[Alex Christensen]

Created attachment 205424 [details]
Patch

[Alex Christensen]

> There is a lot of makeGraphs going on. Could some of this be done lazily like when you click an item?

The calls to makeGraphs don't delay loading significantly, and they are hidden in groups when all subdirectories are collapsed.  I think this would be an unnecessary design change.

[Build Bot]

Comment on attachment 205424 [details]
Patch

Attachment 205424 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/925124

[Build Bot]

Comment on attachment 205424 [details]
Patch

Attachment 205424 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/944171

[Ryosuke Niwa]

Comment on attachment 205424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205424&action=review

> Tools/CodeCoverage/report.html:73
> +            // This is the contents of the images left of directories.

Why don't we use inline svg instead?

> Tools/CodeCoverage/report.html:353
> +                var dataFilename = '../../WebKitBuild/Coverage/' + window.location.search.substring(1) + '.json';

I'm not sure if it's a good idea to hard code the path like this.
Presumably, there are situations in which you want to copy files between different computers, etc...
and still want to be able to open it on the viewer.

> Tools/CodeCoverage/report.html:355
> +                xhr.open('GET', dataFilename, true);
> +                xhr.send();

This is not going to work unless the person explicitly disables local file restriction.
I would have preferred if we had used JSONP or generated a html file that contained this script & JSON.

[Alex Christensen]

Created attachment 205428 [details]
Patch

[Build Bot]

Comment on attachment 205428 [details]
Patch

Attachment 205428 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/912737

[Build Bot]

Comment on attachment 205428 [details]
Patch

Attachment 205428 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/848548

[Alex Christensen]

Created attachment 205512 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 205512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205512&action=review

> Tools/CodeCoverage/results-template.html:356
> +        <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script>

We probably shouldn't call this PeformanceTestsResultsJSON.

[Alex Christensen]

(In reply to comment #17)
> (From update of attachment 205512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205512&action=review
> 
> > Tools/CodeCoverage/results-template.html:356
> > +        <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script>
> 
> We probably shouldn't call this PeformanceTestsResultsJSON.

That's what I get for copying code and only checking to see that it works without actually reading it :)

[Alex Christensen]

Created attachment 205513 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 205428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205428&action=review

Looks good to me

> Tools/CodeCoverage/results-template.html:8
> +                padding: 0px;

Suggestion: Everywhere you have 0, you can drop the units "px", "%", etc. But I'll leave that decision up to you.

> Tools/CodeCoverage/results-template.html:159
> +                xhr.onreadystatechange=function()

Style: '=' => ' = '

> Tools/CodeCoverage/results-template.html:161
> +                    if (xhr.readyState === 4) {

Suggestion: 4 => XMLHttpRequest.DONE

> Tools/CodeCoverage/results-template.html:197
> +                li.className='file';

Style: '=' => ' = '

> Tools/CodeCoverage/results-template.html:234
> +                    children.style.display='';
> +                } else {
> +                    img.src = rightArrow;
> +                    children.style.display='none';

Style: '=' => ' = '

> Tools/CodeCoverage/results-template.html:356
> +        <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script>

Style: This is the only place where you use double quotes intend of single quotes. I'd change it for consistency.

> Tools/Scripts/generate-coverage-data:78
> +$templateHtml =~ s/%PeformanceTestsResultsJSON%/$jsonData/g;

The /g shouldn't be necessary. There will probably only ever be one %PerformanceTestsResultsJSON%, right? Its like a 9MB string.

> Tools/Scripts/generate-coverage-data:85
> +system "open $url";

I'd recommend quoting the path in case anyone has spaces in their path leading up to WebKitBuild.

    system "open '$url'";

Of course using perl exec APIs there is probably a safer way that handles this. Probably:

    system "open", $url;

[Joseph Pecoraro]

Comment on attachment 205513 [details]
Patch

r+ with the comments from the earlier patch.

[Alex Christensen]

Created attachment 205515 [details]
Patch

[Ryosuke Niwa]

Can we see a sample output? (Attach it on the bugzilla)

[Alex Christensen]

Created attachment 205517 [details]
Patch

[Alex Christensen]

Created attachment 205519 [details]
This is an example report.

The data for the example report is from a release build which has some quirks right now (so entire directories seem to be uncovered when they are not), and the links to the files won't work unless you have the same generated files that I do.

[WebKit Commit Bot]

Comment on attachment 205517 [details]
Patch

Clearing flags on attachment: 205517

Committed r152012: <http://trac.webkit.org/changeset/152012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.