Skip to content
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

Original files in HTML reporter #648

Merged
merged 10 commits into from Mar 3, 2017
Merged

Original files in HTML reporter #648

merged 10 commits into from Mar 3, 2017

Conversation

@feugy
Copy link
Contributor

feugy commented Oct 4, 2016

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 feugy changed the title Original files in HTML reporter (WIP) Original files in HTML reporter Oct 4, 2016
'<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>']);

This comment has been minimized.

Copy link
@feugy

feugy Oct 4, 2016

Author Contributor

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

@@ -10,7 +10,7 @@ const internals = {};

exports.method = function (value) {

while (value) {
while ( value ) {

This comment has been minimized.

Copy link
@feugy

feugy Oct 4, 2016

Author Contributor

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

@feugy

This comment has been minimized.

Copy link
Contributor Author

feugy commented Oct 4, 2016

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 geek added the feature label Oct 5, 2016
@geek geek added this to the 11.1.1 milestone Oct 5, 2016
@geek geek self-assigned this Oct 5, 2016
line: num,
column: chunk.column
});
chunk.originalFilename = originalPosition.source.replace(Path.join(process.cwd(), '/').replace(/\\/g, '/'), '');

This comment has been minimized.

Copy link
@geek

geek Oct 5, 2016

Member

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

This comment has been minimized.

Copy link
@feugy

feugy Oct 6, 2016

Author Contributor

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

@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 5, 2016

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

@feugy

This comment has been minimized.

Copy link
Contributor Author

feugy commented Oct 7, 2016

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

This comment has been minimized.

Copy link
Contributor Author

feugy commented Oct 7, 2016

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 ?

const source = ret.source[num];

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, '/');

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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.

@@ -437,10 +453,6 @@ internals.file = function (filename, data, options) {
source: line
};

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

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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

@@ -172,7 +277,7 @@ internals.Reporter.prototype.end = function (notebook) {
internals.findLint = function (lint, file) {

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) {

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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.

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

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

From now, reporter displays both original and generated sources.

This toggle allows to show/hide generated files.

}
.show {
position: relative;
display: inherit;

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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

<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>

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

Link to the corresponding generated file.

<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}}

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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

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

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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)

{ 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 }

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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

@@ -120,8 +120,7 @@ describe('Coverage', () => {
});

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 }

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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.

@@ -211,7 +210,7 @@ describe('Coverage', () => {
const Test = require('./coverage/use-strict.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, '/');

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

To run tests on Windows

@@ -165,7 +165,7 @@ describe('Linters - eslint', () => {

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'));

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

To run tests on Windows

@@ -736,7 +736,7 @@ describe('Reporter', () => {
Lab.report(script, { reporter: 'console', progress: 2, output: false }, (err, code, output) => {

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

All modifications here are needed to run tests on Windows

"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",

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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

}
});
done();
});

This comment has been minimized.

Copy link
@feugy

feugy Oct 8, 2016

Author Contributor

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 feugy force-pushed the feugy:master branch from 9bd11a1 to 3118d12 Oct 8, 2016
@feugy

This comment has been minimized.

Copy link
Contributor Author

feugy commented Oct 17, 2016

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

@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 20, 2016

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

@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 20, 2016

@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

This comment has been minimized.

Copy link
Member

geek commented Feb 23, 2017

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

feugy added 8 commits Oct 4, 2016
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.
@feugy feugy force-pushed the feugy:master branch from daf100c to 1694d79 Feb 25, 2017
@feugy feugy changed the title (WIP) Original files in HTML reporter Original files in HTML reporter Feb 26, 2017
Don't show sloc/percentage on generated file content in Html reporter.
@feugy feugy force-pushed the feugy:master branch from 693e0bf to 324c9ec Feb 26, 2017
@feugy

This comment has been minimized.

Copy link
Contributor Author

feugy commented Feb 26, 2017

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.

@@ -500,7 +500,7 @@ describe('CLI', () => {
expect(result.errorOutput).to.equal('');
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)');

This comment has been minimized.

Copy link
@feugy

feugy Feb 26, 2017

Author Contributor

Due to the new SLOC algorithm

expect(cov.misses).to.equal(19);
expect(cov.hits).to.equal(33);
expect(cov.hits).to.equal(30);

This comment has been minimized.

Copy link
@feugy

feugy Feb 26, 2017

Author Contributor

Due to the new SLOC algorithm

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

This comment has been minimized.

Copy link
@feugy

feugy Feb 26, 2017

Author Contributor

Due to the new SLOC algorithm

expect(cov.misses).to.equal(1);
expect(cov.hits).to.equal(15);
expect(cov.hits).to.equal(10);

This comment has been minimized.

Copy link
@feugy

feugy Feb 26, 2017

Author Contributor

Due to the new SLOC algorithm

@@ -1477,13 +1477,13 @@ describe('Reporter', () => {

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>');

This comment has been minimized.

Copy link
@feugy

feugy Feb 26, 2017

Author Contributor

Due to the new SLOC algorithm

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 27, 2017

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

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 28, 2017

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

@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 28, 2017

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

@feugy

This comment has been minimized.

Copy link
Contributor Author

feugy commented Mar 1, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

feugy commented Mar 1, 2017

@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 ?

@geek geek added this to the 13.0.0 milestone Mar 3, 2017
@geek geek merged commit 44aa4cd into hapijs:master Mar 3, 2017
2 checks passed
2 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tschaub

This comment has been minimized.

Copy link

tschaub commented on lib/coverage.js in 324c9ec Mar 11, 2017

I'm seeing TypeError: Reduce of empty array with no initial value with this call. Which I guess would happen if tree.comments were an empty array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.