Incorrect lines in lcov report when using --transform with --sourcemaps

[bvallee-thefork]

Support plan

• which support plan is this issue covered by? Community
• is this issue currently blocking your project? (yes/no): no
• is this issue affecting a production system? (yes/no): no

Context

• node version: 10.16.3
• module version with issue: 21.0.0
• last module version without issue: none
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application
• any other relevant information:

What are you trying to achieve or the steps to reproduce?

• Have TypeScript source files in src/
• Have tests testing these src/ TypeScript files using --transform node_modules/lab-transform-typescript --sourcemaps
• Example of full test command: NODE_ENV=test npx lab -l --transform node_modules/lab-transform-typescript --sourcemaps -c --coverage-all -r lcov -o lcov.info

What was the result you got?

The lcov.info file contains lines corresponding to the internally transpiled files, not to the original TypeScript files.

What result did you expect?

The lcov.info file should contain the lines from the original TypeScript files.

Additional information

Some related issues were filed (and fixed) for the html output:

• Source maps show wrong lines #338
• Test coverage shows untransformed output #614

[hueniverse]

@geek got time to look into this one?

[hueniverse]

This is not going to be fixed. Going to drop sourcemap support.

[stevendesu]

@hueniverse Is there a reason sourcemap support is being dropped?

[stevendesu]

I've started diving into the @hapijs/lab source code to figure out what it would realistically take to fix the issue of incorrect line counts and the "missing coverage from file(s): null on line(s): , , , ," issue

The primary issue is that the compiled TypeScript files have boilerplate code being added which is not covered by any unit tests. Things like:

var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};

First I noticed the "missing coverage from" messaging was in the console reporter (lib/reporters/console.js) and found I could pretty trivially hide the offending lines by modifying the if (file.sourcemaps) { block -- in particular, I changed if (line.miss) { to if (line.originalFilename && line.miss) { -- this way misses would only be displayed in the output if they mapped to a line in the original source code, and misses that were added by any compilers or transformations would be ignored. Of course there's still the matter of correcting the reported percentage and the total misses count, but this was part of my initial exploration

Since we want the underlying coverage data to be correct and not just how it's output in the console (e.g. if you output to HTML or JSON, those should be correct as well) I started working backwards through lib/coverage.js to find a better solution

This got me looking at internals.file = async function (filename, data, options) {

In this method we do one pass over the transformed file to figure out how many lines contained a statement that was not covered by the test (good so far), but then we immediately increment the hits or misses count. Then after generating the hits and misses counts, we call addSourceMapsInformation, which is only utilized on display.

I have two thoughts at this point:

1. I want to keep reading through addSourceMapsInformation and really make sure I understand internals.file before I make any changes that accidentally break something, but it feels imminently possible to only include a line as a hit or a miss if it exists in the original source file. Technically doing so is ignoring lines of code that were not covered. This therefore shouldn't be the default behavior, but could potentially be enabled with a command-line option (something like -i / --ignore-transformed-files).
2. Regardless, the output "missing coverage from file(s): null on line(s): , , , ," is unhelpful and misleading, so if we can provide a real descriptor of the uncovered lines, that would be better. Maybe something like <sourcefile>-transformed (so that it's clear this was the output after transformation)

[stevendesu]

As an aside, since I figured out what was causing the issue, I was able to resolve it for my project by just changing the TypeScript target to es2017 (which is fine for me since I'm running Node 13)

Update: This only worked for my simple test project. In my real project I'm using TypeORM, which makes heavy use of decorators. Decorators aren't supported by any version of JavaScript, so they create the following polyfill:

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};

This polyfill isn't completely covered by my tests (and likely never will be) so I'm stuck with "missing coverage from file(s): null on line(s): , , ," until a PR is submitted to fix this (possibly using one of the two methods I described in my prior post)

[stevendesu]

For the record, I forked @hapi/lab and added the following near the bottom of the internals.file method. It's not perfect (per the comment), but it works for TypeScript sources:

    // Translate source maps
    if (ret.sourcemaps)
    {
        const mappedSource = {};
        const loadedOriginalSource = {};
        Object.keys(ret.source).forEach(id =>
        {
            const line = ret.source[id];
            if (line.originalFilename)
            {
                // ERROR: If a file came from two original source files (e.g. concatenation)
                //        then we'll only include the first file in line.source and we'll
                //        combine the number of hits
                //
                // Ideally this method needs a way to return more than one file and the files
                // need to be merged by whoever reads the result
                //
                // Although for just TypeScript-to-JavaScript, this is fine
                const originalSource = loadedOriginalSource[line.originalFilename] || Fs.readFileSync(line.originalFilename, 'utf8').split("\n");
                loadedOriginalSource[line.originalFilename] = originalSource;
                let originalLine = mappedSource[line.originalLine] || {
                    source: originalSource[line.originalLine - 1],
                    hits: 0,
                    miss: false
                };
                originalLine.hits += (line.hits || 0);
                originalLine.miss = originalLine.miss || line.miss;
                mappedSource[line.originalLine] = originalLine;
            }
        });
        ret.source = mappedSource;
        ret.sloc = Object.keys(ret.source).length;
        ret.hits = 0;
        ret.misses = 0;
        ret.sourcemaps = false;
        Object.keys(ret.source).forEach(id =>
        {
            if (ret.source[id].miss)
            {
                ret.misses++;
            }
            else
            {
                ret.hits++;
            }
        });
    }

[benoitv-code]

For those looking for a workaround, we're using lab with nyc for coverage (with source-map-support and ts-node/register), it works pretty well.

For decorators, it seems to work reasonably (and surprisingly, it works better when using ts-node/register instead of ts-node/register/transpile-only when running TypeScript files directly).

[hueniverse]

Please try again with the new --typescript flag.