remap-istanbul icon indicating copy to clipboard operation
remap-istanbul copied to clipboard

Typescript extended classes show branch error

Open itslenny opened this issue 8 years ago • 8 comments

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.

itslenny avatar Dec 16 '16 21:12 itslenny

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

DanielRosenwasser avatar Dec 21 '16 22:12 DanielRosenwasser

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?

dylans avatar Dec 21 '16 23:12 dylans

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?

DanielRosenwasser avatar Dec 22 '16 00:12 DanielRosenwasser

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.

dylans avatar Dec 22 '16 01:12 dylans

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

Izhaki avatar Mar 20 '17 21:03 Izhaki

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.

kitsonk avatar Mar 21 '17 18:03 kitsonk

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.

ben8p avatar Dec 13 '17 09:12 ben8p

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?

mohyeid avatar Feb 16 '19 01:02 mohyeid