SitePen / remap-istanbul

A tool for remapping Istanbul coverage via Source Maps
Other
240 stars 79 forks source link

Typescript extended classes show branch error #106

Open itslenny opened 7 years ago

itslenny commented 7 years ago

I'm not 100% sure if this is a remap issue or not. If not let me know what would be the responsible party.

The issue is when you have a class that extends another class and call super() the typescript compiler creates a branch that you cannot access which comes back as a branch failure and can dramatically decrease code coverage if there aren't many other branches in the file.

Typescript example

class Response {
    constructor() { }
}

class HttpResponse extends Response {
    constructor() {
        super();
    }
}

Resulting Javascript

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
var Response = (function () {
    function Response() {
    }
    return Response;
}());
var HttpResponse = (function (_super) {
    __extends(HttpResponse, _super);
    function HttpResponse() {
        return _super.call(this) || this;
    }
    return HttpResponse;
}(Response));

The semi-colon at the end of super(); is highlighted with a "branch not tested" error.

DanielRosenwasser commented 7 years ago

If this is an issue with our source-maps, we can potentially take a look into it for 2.1.5.

dylans commented 7 years ago

Our team at SitePen hasn't run into this issue, but mostly because we avoid the ES6 Class syntax. Based on comments in the Istanbul ticket, it looks like a possible regression from TS 2.0.x to 2.1.x?

DanielRosenwasser commented 7 years ago

Not really a regression - the behavior is intentional because classes need to possibly substitute values that have been explicitly returned from super() calls. This means we do have to capture a new value for _this, as _super.call(...) || this. Most of the time, code will never be executed in that first branch, so Istanbul gets unhappy.

I guess the question is: if we adjusted our source maps in a certain manner, would remap-istanbul be able to effectively ignore the branch and treat it as one entity?

dylans commented 7 years ago

Thanks for clarifying, my regression assumption was based on comments in the other issue. The normal approach for Istanbul to ignore a branch is to add a hint on what to skip, e.g. https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md , but I'm sure there's a better way as that feels hacky and may not really work here anyway.

So I'm sure there's a way, just don't know what that is yet. We'll give it some thought after the holidays.

Izhaki commented 7 years ago

I have had a similar issue.

I'm now convinced that it is best to simply stick to es6 when producing the coverage report - the emitted auxiliary code only reduces coverage with an es5 target. Here is an example how.

Related: https://github.com/Microsoft/TypeScript/issues/13029 https://github.com/Microsoft/TypeScript/issues/13455

kitsonk commented 7 years ago

This is due to the source-map actually mapping back to a specific call. Where other "helper" code gets rolled up and line coverage information gets merged, emitted code branches that are not present in the source code are not intelligently eliminated.

A fix would be to more intelligently determine if branches can be mapped back logically to the source code and if they don't exist (they are only exist in emit land) then their coverage (or lack of coverage) should be dropped out.

ben8p commented 6 years ago

For the one using webpack and TypeScript configured to output ES5, I made a little webpack loader: https://www.npmjs.com/package/ts-es5-istanbul-coverage Using it make sure you won't get the branch marked as not covered.

mohyeid commented 5 years ago

Re my comment below, upgrading to jest-preset-angular 7.0.0-alpha-2 fixed this issue with setting to ES6, incase someone is facing the issue in angular.

Changing the target to ES6, did not help for me. I am still having 50% branch coverage due to the super constructor. Is there any other solution?