Original files in HTML reporter #648

feugy · 2016-10-04T16:22:15Z

This is a first attempt to fix #614.

My idea is to adapt the HTML report to display original files when source maps is present.
It works well, notably to display uncovered lines or statement.

Sloc number, coverage percent and "covered/not covered" counter are wrong for now, because they are computed from generated files, and not from original files.

I had to update the while.js test fixture using latest traceur to make sure we display original source with comment and white spaces, and this explains the small fixes on coverage tests.

feugy · 2016-10-04T16:23:03Z

test/reporters.js

+                    '<div>    while ( </div><div class="miss false" data-tooltip>value ) </div><div>{</div>']);
+                expect(output, 'missed original line not included').to.contains([
+                    '<tr class="miss">',
+                    '<td class="source" data-tooltip>        value &#x3D; false;</td>']);


These are more specific assertion to test particular cases: chunks, missed lines, original filename, comments...

feugy · 2016-10-04T16:24:06Z

test/coverage/while.js

 exports.method = function (value) {

-    while (value) {
+    while ( value ) {


To make a difference between generated and original file, especially for chunks handling.

feugy · 2016-10-04T16:50:42Z

Sorry, for missing coverage, I'll fix it tomorrow.

As I'm on windows, even when using ubuntu bash I can't run all the tests locally.

It hangs, and sometimes even crash with a good old segfault 😀

geek · 2016-10-05T15:43:46Z

lib/coverage.js

+                line: num,
+                column: chunk.column
+            });
+            chunk.originalFilename = originalPosition.source.replace(Path.join(process.cwd(), '/').replace(/\\/g, '/'), '');


Will Path.basename(originalPosition.source); work instead?
https://nodejs.org/api/path.html#path_path_basename_path_ext

I've copy-pasted this from line 404, but I'll give a try, because using basename is definitely more understandable !

geek · 2016-10-05T15:46:26Z

@feugy looks like a great improvement to our reporting. Thanks!

feugy · 2016-10-07T08:31:31Z

Just to let you know: I've found that inlined and external source maps are not reported the same due to my latest modification.

So my TODO list is:

support external sourcemaps
support inlined sourcemaps
fix sloc and coverage counters
write tests to maintain the100% coverage

feugy · 2016-10-07T14:25:57Z

Supporting inline maps involved a huge refactor, but it's nearly done.

@geek while trying to figure out how to get sloc and hits in original files, I found that any lines are considered as "line of code", even comments (data.source includes the entire source content, not just executable lines).

Did I missed something ?

feugy · 2016-10-08T10:44:26Z

lib/coverage.js

    if (position !== originalPosition) {
-        source.originalFilename = originalPosition.source.replace(Path.join(process.cwd(), '/').replace(/\\/g, '/'), '');
+        // Ensure folder separator to be url-friendly
+        source.originalFilename = originalPosition.source && originalPosition.source.replace(/\\/g, '/');


Sourcemaps always expect path to use / separator, as URLs does, so we need to do the replacement.
Removing process.cwd() is useless as the sourcemap won't contain it.

feugy · 2016-10-08T10:45:13Z

lib/coverage.js


-        if (options.sourcemaps) {
-            internals.addSourceMapsInformation(ret, +num);
-        }


Move to the end of the loop, because we need to have the chunk before adding sourcemaps information

feugy · 2016-10-08T10:47:01Z

lib/reporters/html.js


    for (let i = 0; i < lint.length; ++i) {
-        if (lint[i].filename.slice(-file.filename.length) === file.filename && lint[i].errors.length) {
+        if (lint[i].filename.replace(/\\/g, '/').slice(-file.filename.length) === file.filename && lint[i].errors.length) {


lint.filename is platform dependant, which is ok (notably when we fix lint issues), but file.filename always use /, so we need to to the replacement.

feugy · 2016-10-08T10:47:47Z

lib/reporters/html/partials/cov.html

+    <div id="filters">
+      <input type="checkbox" checked="" onchange="filter(this)" value="generated" id="show-generated">
+      <label for="show-generated">Show generated files</label>
+    </div>


From now, reporter displays both original and generated sources.

This toggle allows to show/hide generated files.

feugy · 2016-10-08T10:48:22Z

lib/reporters/html/partials/css.html


    .show {
-        position: relative;
+        display: inherit;


Otherwise, hidden generated files take some height in right-menu.

feugy · 2016-10-08T10:49:02Z

lib/reporters/html/partials/file.html

-<div class="file">
-    <h2 id="{{this.filename}}">{{this.filename}}</h2>
+<div class="file {{#if this.sourcemaps}}show generated{{/if}}">
+    <h2 id="{{this.filename}}">{{this.filename}} {{#if this.generated}}(transformed to <a href="#{{this.generated}}">{{this.generated}})</a>{{/if}}</h2>


Link to the corresponding generated file.

feugy · 2016-10-08T10:49:43Z

lib/reporters/html/partials/line.html

+  {{else}}
+  <td class="source"{{#if this.miss}} data-tooltip{{/if}}>{{this.source}}</td>
+  {{/if}}
+  {{#if this.originalFilename}}<td class="original-line" data-tooltip="{{this.originalFilename}}"><a href="#{{this.originalFilename}}__{{this.originalLine}}">{{this.originalLine}}</a></td>{{/if}}


If displaying a generated file, link to original file and line.

feugy · 2016-10-08T10:51:32Z

lib/reporters/html/partials/line.html

-            <td class="sourcemaps line" data-tooltip>{{this.originalLine}}</td>
-        {{/if}}
-    </tr>
-{{/if}}


This is a condensed version of previous code, to limit code duplication, although I agree it's less readable.
I've added a unic ID on each line to allow reference from generated files.

One possible improvement is to remove this. which are optional (handlebars implicitly look into this)

feugy · 2016-10-08T10:52:02Z

test/coverage.js

-            { filename: 'test/coverage/while.js', lineNumber: '5', originalLineNumber: 11 },
-            { filename: 'test/coverage/while.js', lineNumber: '6', originalLineNumber: 12 }
+            { filename: 'while.js', lineNumber: '4', originalLineNumber: 13 },
+            { filename: 'while.js', lineNumber: '5', originalLineNumber: 14 }


Due to different test fixture, compiled with babel instead of traceur.

feugy · 2016-10-08T10:55:36Z

test/coverage.js

        expect(missedLines).to.include([
-            { filename: './while.js', lineNumber: '5', originalLineNumber: 11 },
-            { filename: './while.js', lineNumber: '6', originalLineNumber: 12 }
+            { filename: 'while.js', lineNumber: '3', originalLineNumber: 8 }


The fact that only one missing line is detected is a possible bug.

I've replaced the sourcemaps-inline.js with a newer version processed by babel, and compacted into a single line:

'use strict';// Load modules // Declare internals const internals={};exports.method=function(value){while(value){value=false;}return value;}; //# sourceMappingURL=data:application/json;base64,...

Coverage detects that while(value) is always false, but doesn't detect that value=false is never reached.

This only happens when generated file is compacted, I'll dig that a little more.

EDIT
The line is properly detected as missed, but because generated code is only on a single line, we need to checked missed chunks also.

feugy · 2016-10-08T10:56:09Z

test/coverage.js

        expect(Test.method.toString()).to.not.contain('13'); // This is the line of the inner use strict

-        const testFile = Path.join(__dirname, 'coverage/use-strict.js');
+        const testFile = Path.join(__dirname, 'coverage/use-strict.js').replace(/\\/g, '/');


To run tests on Windows

feugy · 2016-10-08T10:56:25Z

test/linters.js

        Fs.writeFileSync = (path, output) => {

-            expect(path).to.endWith('test/lint/eslint/fix/success.js');
+            expect(path).to.endWith(Path.join('test', 'lint', 'eslint', 'fix', 'success.js'));


To run tests on Windows

feugy · 2016-10-08T10:56:44Z

test/reporters.js

                });
            });

            Lab.report(script, { reporter: 'console', progress: 2, output: false }, (err, code, output) => {


All modifications here are needed to run tests on Windows

feugy · 2016-10-08T13:40:26Z

package.json

-    "test-cov-html": "./bin/_lab -fL -r html -m 3000 -o coverage.html",
-    "lint": "./bin/_lab -d -f -L",
+    "test-cov-html": "node bin/_lab -fL -r html -m 3000 -o coverage.html",
+    "lint": "node bin/_lab -d -f -L",


Unfortunately, Windows' cmd and Powershell doesn't understand the ./ syntax.
By using node invokation, we ensure a portable command.

feugy · 2016-10-08T13:43:41Z

test/leaks.js

+            }
+        });
+        done();
+    });


On Windows, global keys can actually be deleted, which makes those test not runnable.
I refactored them to:

automatically delete globals set to 1, and not delete the pre-existing values

check all counter globals existence and absence.

feugy · 2016-10-17T06:45:50Z

Hello @geek, I would like to have your opinion on the current state, and I have a question

geek · 2016-10-20T19:38:09Z

@feugy I think you found a bug that we are counting comments towards lines of code :/

geek · 2016-10-20T19:50:35Z

@feugy everything here looks like really good changes. After the fix for the sloc, ping me and I will give it one final review before merging.

Awesome work!

geek · 2017-02-23T01:05:16Z

@feugy any luck finding time to look at this PR again... it looks very close to being complete.

Sloc and coverage display are broken for now

HTML reporter now displays original and generated files (a toggle allows to hide them). Hits, sloc and misses are still broken in original files.

Don't show sloc/percentage on generated file content in Html reporter.

feugy · 2017-02-26T18:02:22Z

Hello @geek !
It's been a long time since my last contribution, all due to a lot corporate work 😄

For Html reporter, I've finally decided to hide sloc counter / coverage percentage on original files, to only show them on generated files. In fact, sourcemaps allow to reverse the original file content, but can't be blindly used to recompute SLOC & coverage.

But this leads me to another (and way more important) fix.
Up to now, lab was considering all lines when computing SLOC indicator, which was easier but not aligned with common definitions [1], [2]:

The common definition of physical SLOC describes them as the lines that do not contain blanks or
comments

I've fixed this, to rely only on the AST output. Now SLOC is:

Total number of generated lines - blank lines - commented lines
Without considering actual coverage (hits and misses).

All this works (I've added another test), but has an unexpected impact: it mechanically lower the overall coverage percentage.
This may be considered as a breaking change, because some files that were above coverage threshold might be bellow it now that we don't consider comments in the total.

I leave you the final appreciation, I can easily revert the SLOC computation to what it was before.

feugy · 2017-02-26T18:04:31Z

test/cli.js

            expect(result.code).to.equal(1);
            expect(result.output).to.contain('1 tests complete');
-            expect(result.output).to.contain('Coverage: 92.86% (1/14)');
+            expect(result.output).to.contain('Coverage: 90.00% (1/10)');


Due to the new SLOC algorithm

feugy · 2017-02-26T18:05:02Z

test/coverage.js

+        expect(cov.sloc).to.equal(49);
        expect(cov.misses).to.equal(19);
-        expect(cov.hits).to.equal(33);
+        expect(cov.hits).to.equal(30);


Due to the new SLOC algorithm

feugy · 2017-02-26T18:05:19Z

test/coverage.js

+        expect(cov.sloc).to.equal(12);
        expect(cov.misses).to.equal(0);
-        expect(cov.hits).to.equal(16);
+        expect(cov.hits).to.equal(12);


Due to the new SLOC algorithm

feugy · 2017-02-26T18:05:22Z

test/coverage.js

+        expect(cov.sloc).to.equal(13);
        expect(cov.misses).to.equal(1);
-        expect(cov.hits).to.equal(15);
+        expect(cov.hits).to.equal(10);


Due to the new SLOC algorithm

feugy · 2017-02-26T18:05:50Z

test/reporters.js

                expect(err).not.to.exist();
                expect(output).to.contain('<div class="stats medium">');
-                expect(output).to.contain('<span class="cov medium">71.43</span>');
+                expect(output).to.contain('<span class="cov medium">66.67</span>');


Due to the new SLOC algorithm

geek · 2017-02-27T15:51:34Z

@feugy looks good so far, I am going to take a closer look later and merge/release.

geek · 2017-02-28T17:48:37Z

@feugy do we need the report-*.html files in the root?

geek · 2017-02-28T17:51:34Z

@feugy if you run the html report on lab it will report 103.31% coverage for lib/index.js

feugy · 2017-03-01T08:05:21Z

@geek about the report.html, its a mistake (test outputs I've forgot to delete). About the coverage, I'm more concerned: there must be an some lines I've considered as commented and that were hit.

feugy · 2017-03-01T18:23:48Z

@geek, nicely spotted !
The bug was pretty obvious: blank lines within comments were subtracted twice: one as commented lines, one as blank lines.

It was the opportunity for me to refactor the code, and use only the AST to detect commented lines.
This solution is simpler and more reliable, because detecting comments during coverage pass is tricky.

Can you please give a try ?

lock · 2020-01-09T07:37:11Z

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

feugy changed the title ~~Original files in HTML reporter~~ (WIP) Original files in HTML reporter Oct 4, 2016

feugy commented Oct 4, 2016

View reviewed changes

geek added the feature New functionality or improvement label Oct 5, 2016

geek added this to the 11.1.1 milestone Oct 5, 2016

geek self-assigned this Oct 5, 2016

geek reviewed Oct 5, 2016

View reviewed changes

feugy commented Oct 8, 2016

View reviewed changes

feugy force-pushed the master branch from 9bd11a1 to 3118d12 Compare October 8, 2016 13:52

feugy added 8 commits February 25, 2017 11:49

Display original files in HTML reporter

c3af751

Sloc and coverage display are broken for now

Remove verbose flag

34c40a6

Fix some test to be runnable on Windows

fc01555

HTML reporter support external & inline sourcemaps

3463b9f

HTML reporter now displays original and generated files (a toggle allows to hide them). Hits, sloc and misses are still broken in original files.

Fix coverage issues

8aa5bd7

Make ALL test work under Windows

ceea41e

Fix test timeout

4588d1a

Fix failing test on windows

1694d79

feugy force-pushed the master branch from daf100c to 1694d79 Compare February 25, 2017 10:49

feugy changed the title ~~(WIP) Original files in HTML reporter~~ Original files in HTML reporter Feb 26, 2017

Don't consider comments when computing SLOC

324c9ec

Don't show sloc/percentage on generated file content in Html reporter.

feugy force-pushed the master branch from 693e0bf to 324c9ec Compare February 26, 2017 17:59

feugy commented Feb 26, 2017

View reviewed changes

geek added the breaking changes Change that can breaking existing code label Feb 27, 2017

Fix sloc computation & remove test outputs

3305626

geek added this to the 13.0.0 milestone Mar 3, 2017

geek merged commit 44aa4cd into hapijs:master Mar 3, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020

Original files in HTML reporter #648

Original files in HTML reporter #648

Uh oh!

Conversation

feugy commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feugy commented Oct 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geek commented Oct 5, 2016

Uh oh!

feugy commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feugy commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feugy Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feugy Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feugy commented Oct 17, 2016

Uh oh!

geek commented Oct 20, 2016

Uh oh!

geek commented Oct 20, 2016

Uh oh!

geek commented Feb 23, 2017

Uh oh!

feugy commented Feb 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geek commented Feb 27, 2017

Uh oh!

feugy commented Oct 7, 2016 •

edited

Loading

feugy commented Oct 7, 2016 •

edited

Loading

feugy Oct 8, 2016 •

edited

Loading

feugy Oct 8, 2016 •

edited

Loading

feugy commented Mar 1, 2017 •

edited

Loading