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

Typescript extended classes show branch error #106

Open
itslenny opened this issue Dec 16, 2016 · 8 comments
Open

Typescript extended classes show branch error #106

itslenny opened this issue Dec 16, 2016 · 8 comments

Comments

@itslenny
Copy link

itslenny commented Dec 16, 2016

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

DanielRosenwasser commented Dec 21, 2016

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

@dylans
Copy link
Contributor

dylans commented Dec 21, 2016

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

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
Copy link
Contributor

dylans commented Dec 22, 2016

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 dylans added this to the 0.9.0 milestone Jan 11, 2017
@dylans dylans modified the milestones: 0.9.0, 0.10.0 Feb 22, 2017
pelotom added a commit to runtypes/runtypes that referenced this issue Mar 20, 2017
@Izhaki
Copy link

Izhaki commented Mar 20, 2017

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:
microsoft/TypeScript#13029
microsoft/TypeScript#13455

@kitsonk
Copy link
Contributor

kitsonk commented Mar 21, 2017

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

ben8p commented Dec 13, 2017

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

mohyeid commented Feb 16, 2019

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants