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

why is a constructor parameter considered a branch and not covered? #70

Open
daBishMan opened this issue Jun 23, 2017 · 58 comments
Open

Comments

@daBishMan
Copy link

Hello all,

Thanks for great work, and sorry if this is not the right forum to ask this question.
image

As you can see from the image above, the constructor params are considered branch and are not covered, the rest of the class is 100% expect the constructor.

Can you tell me why it is counting the constructor as branches, and how can I fix it, please?

@ChrisLebbano
Copy link

I am having the same issue. I found some fix that uses comments to ignore lines of code like /* istanbul ignore next */ but these do not work at all.

@EugeneSnihovsky
Copy link

EugeneSnihovsky commented Jun 29, 2017

+1
Have same issue with comma and /* istanbul ignore next */ not helped.
screen with example

@bashirsouid
Copy link

bashirsouid commented Jul 7, 2017

Edit: we are having a similar issue with decorators. I originally posted here but then felt it is different enough to be a separate issue. I created issue #72 .

@bcoe
Copy link
Member

bcoe commented Jul 22, 2017

@daBishMan @EugeneSnihovsky I will make sure we have a test case around constructors in the tests for the instrumenter.

Unfortunately, as with #72, when a transpiler is being used to shim language features the generated code can sometimes introduce branches that are difficult to cover -- what does the constructor ultimately look like, when run through the TypeScript compiler?

@troythacker
Copy link

I am experiencing the same issues as @EugeneSnihovsky and @daBishMan.

One of the failing constructors is simple:

public constructor(private renderer: Renderer2) { }

and the transpiled javascript is:

function SearchComponent(renderer) {
   this.renderer = renderer;
}

but when running coverage, I get the following result:

screenshot 2017-09-20 06 34 40

@Remco75
Copy link

Remco75 commented Sep 26, 2017

I have exactly the same in an angular4 env.
the classes are decorated with the angular @Injectable decorator. Maybe that causes this?

@bobbajs
Copy link

bobbajs commented Nov 14, 2017

+1

I tried using /* istanbul ignore next */ but it doesn't work for the comma syntax for constructor. Anyone found a workaround?

@fc1943s
Copy link

fc1943s commented Dec 6, 2017

Having this problem too =( wrong lines/coverage for typescript

@anubhavgupta
Copy link

Any ETA on this issue please?

@siropeich
Copy link

I found a (quite ugly) workaround by adding any to the type of the parameters causing it:

constructor(@inject(TYPES.IDataProvider) private dataProvider: IDataProvider | any) {
}

@otroboe
Copy link

otroboe commented Feb 20, 2018

I'm interested by a solution too, I'm working with NestJS, and I have the same issue.

@lvalencia
Copy link

lvalencia commented Feb 24, 2018

Same issue here +1 NestJS

edit I was able to fix my coverage issue

@otroboe
So for a controller you're gonna add an inject for the variable you're adding and an (or any)
both are important because otherwise your specs are gonna break when the TestModule tries to create it, you'll get this error

" Error: Nest can't resolve dependencies of the ShowController (?). Please verify whether [0] argument is available in the current context. "

if you only add | any

so it'll look something like this

import { Get, Res, Controller } from '@nestjs/common';
import { SomeService } from './some.service';
import { Inject } from '@nestjs/common/utils/decorators/inject.decorator';

@Controller('somes')
export class SomeController {
    constructor(@Inject(SomeService) private readonly somesService: SomeService | any) {}

    @Get()
    async index(@Res() response) {
        const somes = this.somesService.all();
        response.send({ somes });
    }
}

for a service, the | any is enough and it'll look something like this

import { Component } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';
import { Some } from './some.entity';

@Component()
export class SomeService {
    constructor(
        @InjectRepository(Some)
        private readonly someRepository: Repository<Some> | any,
    ) {}

    async all(): Promise<Some[]> {
        return await this.someRepository.find();
    }
}

It may be the case that what you lose is type-ahead and type-checking, (I didn't have that issue) but you get 100% code-coverage in return. So it's up to your discretion.

@ericzon
Copy link

ericzon commented Feb 25, 2018

+1 I'm using NestJs too
I prefer to wait until there's a good solution for this, I appreciate so much my type-ahead & checking...

@tedp
Copy link

tedp commented Mar 27, 2018

I'm also having the same issue when using Jest with Angular 5 and Typescript. Is this something that is being addressed as of now or at least in the pipeline to be fixed?

@pie6k
Copy link

pie6k commented May 25, 2018

image

I'm having similar issue. No idea what is not covered here.

@dWhisper
Copy link

dWhisper commented Jun 7, 2018

Seeing the same issue, making the branch metrics on the coverage report unreliable.

@mcblum
Copy link

mcblum commented Jun 12, 2018

We're seeing the same thing on Angular 5 + Jest. It's not every single line in the constructor, which is weird. Sometimes there will be three things being injected and it will just be one line or so that's showing as uncovered.

@teclone
Copy link

teclone commented Jul 1, 2018

Hello guys esp, @pie6k . I can confirm that i just discovered a fix for this. I was working on an open source project here forensic-js when i bumped into this problem, I had to dig in and thankfully, i was able to find a fix.

Here is my Report and Fix

First of all, this issue arises when your class extends another class. It does not happen if your class does not extend another class.

What then is the Cause?

The cause is that when a class extends another class, The class constructor is obligated to call super() before any other code. To maintain this, the transpiler introduces a branching by injecting the code below into your constructor while trying to locate the super class this object.

var _this = possibleConstructorReturn(this, (_DOMTokenList.__proto__ ||
Object.getPrototypeOf(_DOMTokenList)).call(this, null, true, true));

As you can see, it tries to access it using the deprecated __proto__ property, else it uses the standard Object.getPrototypeOf(). Note that _DOMTokenList is the class name of the module i was working on.

What Then is the Fix?

The fix is creating an instance of your class two times to statisfy those conditions, and thankfully, the __proto property is both a getter and a setter.

Here is how i did it:

let instance = null,
proto = MyClassName.__proto__;

//lets satisfy condition 1
MyClassName.__proto__ = null;
//put in try catch because it throws error
try {
    instance = new MyClassName(...args);
}
catch(ex){}

//lets satisfy condition two, set back the proto
MyclassName.__proto__ = proto;
instance = new MyClassName(...args);

Thats it. you can wrap this into a function and make it reusable across tests. Hope this makes someone smile.... lol.

If you are using jshint for linting like me, use the proto property to prevent jshint from reporting this as deprecated.

@mikeesouth
Copy link

I'm having the same problem but I can't get your fix to work @harrison-ifeanyichukwu
Any other tips or suggested workarounds for this issue would be appreciated.

@teclone
Copy link

teclone commented Jul 11, 2018

@mikeesouth, this issue all depends on the nature of your constructor, and a better way for me to offer any other suggestion is by having a look at the final build. if you don't mind, you can paste the line of code that is not covered here, or my email harrisonifeanyichukwu@gmail.com.. if it is an open source project, i can replicate it from my end as well if given the link. thanks.

Also note that the transpiler used in my case is babel. if your are working with typescript transpiler, there may be some differences, that is why i need to replicate your issue from my own end.

@rhogeranacleto
Copy link

Hi @harrison-ifeanyichukwu, how are you doing?

I'm having the same problem as well. Coverage report doesn't give a real feedback, the branch coverage in reports is 88%, but I've tested 100% of my code. I created a simulation in the following link https://github.com/rhogeranacleto/branch-not-covered-simulation, have a look into the modules/gate gate.controller.ts:

image

And in modules/gate gate.entity.ts as well:
captura de tela de 2018-07-19 16-37-58

Do you have any suggestion?

@teclone
Copy link

teclone commented Jul 28, 2018

Hi @rhogeranacleto , hope you are doing great,

I have been looking into your issue, I think your problem all boils down to the use of decorators. Though it is different from my own problem and suggested fix, below is my suggestion based on my findings.

The decorators introduces branching and your tests have to cover this scenerios

Concerning getAll() method on line 14, in file gates.controller.ts:

The @Get() decorator takes an optional path argument. Below is the prototype from nestjs:

export declare const Get: (path?: string) => MethodDecorator;

This introduces a branch. Therefore, your test should cover both situations, you should test when a path argument is supplied and when a path argument is not supplied.

The second problem on the same line is that the @Query(PaginationPipe)
takes a comma separated list of Type<PipeTransform> or PipeTransform arguments. below is the prototype from nestjs:

export declare function Query(...pipes: (Type<PipeTransform> | PipeTransform)[]): any;

Your test should cover both scenerio. test when the argument is of type Type<PipeTransform>
and when the argument is of type PipeTransform, also test when there are no arguments too.

For other similar lines, use your text editor's go to definition functionality, you will see the prototype of these decorators from your nestjs node module. and fix the issues using my logic here. Thanks

I hope this rectifies your case. I would really appreciate your feedback.

@rhogeranacleto
Copy link

rhogeranacleto commented Jul 30, 2018

Who is passing by branch not covered in randoms statements just add ignoreCoverageForAllDecorators described on ts-jest documentation. Will works for the most of cases I guess.
About the others branch not covered, the decorators are making it, as @harrison-ifeanyichukwu reported. We are trying to find some solution.
Hope this helps! 🙂

@itslenny
Copy link

@rhogeranacleto is this still working for you? I tried it, but it didn't seem to change anything.

@rhogeranacleto
Copy link

@itslenny in some cases yes. As you can see on print screens below:

class 1 before config class 2 before config
class with no coverage controller with no coverage
class 1 after config class 2 after config
class with coverage controller with coverage

As we can see, the only thing I can't fix it is the coverage on dependency injection. Anyone with a workaround would be very pleasurable. 🙃 🙃

@timtrinidad
Copy link

timtrinidad commented Sep 14, 2018

I ran into this issue with ts-jest and inversify.

After some debugging, I found that tsc transpiled the @provides or @fluentProvide decorators into the following

MysqlClient = MysqlClient_1 = __decorate([
    inversify_binding_decorators_1.fluentProvide(MysqlClient_1)
        .inSingletonScope()
        .done(),
    __metadata("design:paramtypes", [connection_1.MysqlConnection])
], MysqlClient);

but for some reason ts-jest transpiled it into the following:

MysqlClient = MysqlClient_1 = /* istanbul ignore next */__decorate([
    inversify_binding_decorators_1.fluentProvide(MysqlClient_1)
        .inSingletonScope()
        .done(),
    __metadata("design:paramtypes", [typeof (_a = typeof connection_1.MysqlConnection !== "undefined" && connection_1.MysqlConnection) === "function" ? _a : Object])
], MysqlClient);

Looking at the source map visualization, it's that typeof ternary that's causing an uncovered branch. I wasn't sure why it only showed up when transpiled with ts-jest and not tsc.
image

In any case, we solved this by setting emitDecoratorMetadata: false as a compiler option for ts-jest.

// jest.config.js
module.exports = {
  // ... other settings ...
  globals: {
    'ts-jest': {
      tsConfigFile: 'tsconfig.jest.json',
    },
  },
};
// tsconfig.jest.json
{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "emitDecoratorMetadata": false
  }
}

@luchsamapparat
Copy link

luchsamapparat commented Oct 22, 2019

I have the same issues with an up-to-date Nx Workspace

@nicolaspearson
Copy link

nicolaspearson commented Feb 10, 2020

Setting the tsConfig and preset in the jest.config.js file worked for me:

module.exports = {
  ...
  globals: {
    'ts-jest': {
      tsConfig: 'tsconfig.json',
    },
  },
  preset: 'ts-jest',
  ...
}

@walterspieler
Copy link

walterspieler commented Mar 9, 2020

I'm interested by a solution too, I'm working with NestJS, and I have the same issue.

@otroboe Did find a workaround for NestJS? I have the same issue but only on one service on the whole app.

@otroboe
Copy link

otroboe commented Mar 9, 2020

@walterspieler
I don't remember I'm sorry, it's been a long time! I still have a repo, but it's quite simple and old and I don't remember if I fixed the issue afterall
https://github.com/otroboe/nestjs-playground

askirmas pushed a commit to askirmas/patterning that referenced this issue Mar 21, 2020
@vikrambhatla
Copy link

vikrambhatla commented May 22, 2020

Same issue here +1 NestJS

edit I was able to fix my coverage issue

@otroboe
So for a controller you're gonna add an inject for the variable you're adding and an (or any)
both are important because otherwise your specs are gonna break when the TestModule tries to create it, you'll get this error

" Error: Nest can't resolve dependencies of the ShowController (?). Please verify whether [0] argument is available in the current context. "

if you only add | any

so it'll look something like this

import { Get, Res, Controller } from '@nestjs/common';
import { SomeService } from './some.service';
import { Inject } from '@nestjs/common/utils/decorators/inject.decorator';

@Controller('somes')
export class SomeController {
    constructor(@Inject(SomeService) private readonly somesService: SomeService | any) {}

    @Get()
    async index(@Res() response) {
        const somes = this.somesService.all();
        response.send({ somes });
    }
}

for a service, the | any is enough and it'll look something like this

import { Component } from '@nestjs/common';
import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';
import { Some } from './some.entity';

@Component()
export class SomeService {
    constructor(
        @InjectRepository(Some)
        private readonly someRepository: Repository<Some> | any,
    ) {}

    async all(): Promise<Some[]> {
        return await this.someRepository.find();
    }
}

It may be the case that what you lose is type-ahead and type-checking, (I didn't have that issue) but you get 100% code-coverage in return. So it's up to your discretion.

Thanks for your answer. It works for me. I am able to get 100% coverage 👍

export class UserService {
    constructor(@Inject(HttpClient) private httpClient: HttpClient | any) {}

}

@ajec-dev
Copy link

Is there any update on this issue? Still seeing constructor coverage gaps with typescript: "3.8.3", ts-jest: "26.0.0", and jest-preset-angular: "8.1.3".

image

This issue seems to present on HttpClient injection, and the | any solution is less than ideal in this case, as it prevents providing an expected type for the HTTP response.

image

@alecgibson
Copy link

For anyone using Webpack with istanbul-instrumenter-loader, we solved this by basically copying the ts-jest solution and adding a little wrapper:

const istanbulInstrumenter = require('istanbul-instrumenter-loader');

exports.default = function(source, sourceMap) {
  // Ignore decorators on methods and properties
  source = source.replace(/(tslib.+\.__decorate)/g, '/* istanbul ignore next */$1');
  // When constructor parameters have decorated properties (eg @inject), TS adds
  // a typeof branch check, which we don't want to instrument
  source = source.replace(/(typeof \(_\w\s*=)/g, '/* istanbul ignore next */$1');

  istanbulInstrumenter.apply(this, [source, sourceMap]);
}

Then you replace your usage of the loader in your Webpack config:

{
    test: /\.[tj]sx?$/,
    use: {
      loader: path.resolve(__dirname, 'webpack', 'istanbul-instrumenter.js'),
      options: {
        esModules: true,
      },
    },
}

@stevengunneweg
Copy link

I also encountered this issue. After much searching it turned out that the link to my tsconfig was incorrect.
In my jest.config.js I had
tsconfig: '<rootDir>/src/tsconfig.spec.json'
When I changed it to
tsconfig: 'src/tsconfig.spec.json'
the issues were resolved. Aparently "<rootDir>" can't be used here.
I guess it was the same issue as #70 (comment) addresses in the end.
I still need to keep isolatedModules to "false" unfortunately.

@lukaVarga
Copy link

In our case the report was incorrect due to having offloaded the type-checking of TS files to a separate node process.

@vad3x
Copy link

vad3x commented Nov 22, 2021

I've ended up with this workaround (similar to what @alecgibson has). Works for anyone who use bare ts-jest:

// fix-istanbul-decorators.js

const { default: tsJest } = require('ts-jest');

module.exports = fixIstanbulDecoratorCoverageTransformer();

function fixIstanbulDecoratorCoverageTransformer() {
  const transformer = tsJest.createTransformer();
  const process = transformer.process.bind(transformer);
  transformer.process = (...args) => {
    let result = process(...args);
    // Ignore decorators on methods and properties
    result = result.replace(
      /__decorate/g,
      '/* istanbul ignore next */__decorate',
    );

    // When constructor parameters have decorated properties (eg @inject), TS adds
    // a typeof branch check, which we don't want to instrument
    result = result.replace(
      /(?<=__metadata\("design:paramtypes".*?)(typeof \(_\w\s*=)/g,
      '/* istanbul ignore next */$1',
    );

    return result;
  };

  return transformer;
}
// jest.config.js

...

  globals: {
    'ts-jest': {
      isolatedModules: true,
    },
  },
  transform: {
    '\\.(ts|tsx)$': '<rootDir>/fix-istanbul-decorators.js',
  },
...

Ref: kulshekhar/ts-jest#1166 (comment)

@micalevisk
Copy link

micalevisk commented May 3, 2022

for jest@28 the solution above will now be the following:

// fix-istanbul-decorators.js

// See https://github.com/istanbuljs/istanbuljs/issues/70#issuecomment-975654329
const { default: tsJest } = require('ts-jest');

module.exports = fixIstanbulDecoratorCoverageTransformer();

function fixIstanbulDecoratorCoverageTransformer() {
  const transformer = tsJest.createTransformer();
  const process = transformer.process.bind(transformer);
  transformer.process = (...args) => {
    let result = process(...args);
    // Ignore decorators on methods and properties
    result.code = result.code.replace(/__decorate/g, '/* istanbul ignore next */__decorate');

    // When constructor parameters have decorated properties (eg @inject), TS adds
    // a typeof branch check, which we don't want to instrument
    result.code = result.code.replace(
      /(?<=__metadata\("design:paramtypes".*?)(typeof \(_\w\s*=)/g,
      '/* istanbul ignore next */$1',
    );

    return result;
  };

  return transformer;
}

see https://jestjs.io/docs/upgrading-to-jest28#transformer

@mandarini
Copy link

mandarini commented Jul 15, 2022

We are facing the same issue, and this happens only when we're using

    'ts-jest': {
      isolatedModules: true,
    },

The solutions provided in the comments (eg. this and this) do not seem to have any effect.

@micalevisk @vad3x these solutions do not seem to have an effect on my end.

This is how I solved the issue, in an Angular app. I had to specifically add /* istanbul ignore next */ before every single constructor parameter, and add /* istanbul ignore next */ before the constructor, too.

@michaelfaith
Copy link

michaelfaith commented Sep 16, 2022

Experiencing this now after upgrading from Angular 14.1 to Angular 14.2 (using Yarn workspace, not sure about npm). Any suggestions? I'm not using isolatedModules, so I don't know that @mandarini's solution applies to me. And I'm using jest-preset-angular rather than bare ts-jest, so I can't really change the transformer out like the other suggestions.

@mandarini
Copy link

Hi @michaelfaith ! My hacky solution does use jest-preset-angular! I just import it in the hacky transformation. Again, I'm not saying it's a solution, since it's hacky, but rather a workaround. It would be nice to have an actual solution from istanbul...

@michaelfaith
Copy link

michaelfaith commented Sep 19, 2022

Thanks. I guess I thought your solution was focused on solving this for people using isolatedModules turned on, which I don't have, currently. I'll give it a try, though. And yeah, considering this is a 5 year old issue (which makes it even weirder that I'd only just now encounter it on a minor version update of Angular), I'm left with little hope that istanbul will actually do anything to fix it.

@matthewtquinn1
Copy link

Recently moved from Angular 13 to Angular 14.4.0 and this is now a problem for our code coverage.
Angular: 14.4.0
jest: 28.1.3
jest-preset-angular: 12.2.3
ts-jest: 28.0.8

Is there a fix available for this? All of my constructors are flagging up now as branches that are not covered

@michaelfaith
Copy link

michaelfaith commented Dec 7, 2022

Recently moved from Angular 13 to Angular 14.4.0 and this is now a problem for our code coverage. Angular: 14.4.0 jest: 28.1.3 jest-preset-angular: 12.2.3 ts-jest: 28.0.8

Is there a fix available for this? All of my constructors are flagging up now as branches that are not covered

What finally solved this issue for us in Angular 14.2+ was removing "emitDecoratorMetadata": true, from the tsconfig. Apparently it was no longer needed after v12, and for whatever reason was causing this issue in testing after 14.2

@matthewtquinn1
Copy link

Yep, that solved it! Thanks very much @michaelfaith 😃

@mandarini
Copy link

Nice @michaelfaith ! Is this valid even for isolatedModules: true?

@michaelfaith
Copy link

michaelfaith commented Dec 7, 2022

I haven't tried it with isolatedModules, and last time I turned that setting on, 80% of my tests failed. So I can't really test that in my proj.

@dotmobo
Copy link

dotmobo commented Jan 5, 2023

Thanks @michaelfaith, it works with Angular 14 !

@sans-jmansfield
Copy link

This is still an issue when emitDecoratorMetadata: false is not a viable option for a given implementation and changing Angular versions is not relevant, e.g. when testing a NestJS application.

image

@AydenOwl
Copy link

Hi guys,

First: thank you @micalevisk for your file ! :) For my project, it was just missing one thing to work properly and to remove this warning: [(WARN) Define ts-jestconfig underglobals is deprecated](https://stackoverflow.com/questions/75955311/warn-define-ts-jest-config-under-globals-is-deprecated).

@sans-jmansfield I think it could solve your problem, I'm using Nest too and all is good now.

My configuration:
"jest": "29.1.0"
"ts-jest": "29.1.0",
"@nestjs/common": "^8.0.0"

fix-istanbul-decorator.js:

// eslint-disable-next-line @typescript-eslint/no-var-requires
const { default: tsJest } = require('ts-jest')

module.exports = fixIstanbulDecoratorCoverageTransformer()

function fixIstanbulDecoratorCoverageTransformer() {
  // The option of isolatedModules was necessary for me
  const transformer = tsJest.createTransformer({ isolatedModules: true })

  const process = transformer.process.bind(transformer)

  transformer.process = (...args) => {
    let result = process(...args)

    // Ignore decorators on methods and properties
    result.code = result.code.replace(/__decorate/g, '/* istanbul ignore next */__decorate')

    // When constructor parameters have decorated properties (eg @inject), TS adds
    // a typeof branch check, which we don't want to instrument
    result.code = result.code.replace(
      /(?<=__metadata\("design:paramtypes".*?)(typeof \(_\w\s*=)/g,
      '/* istanbul ignore next */$1'
    )

    return result
  }

  return transformer
}

jest.config.js:

module.exports = {
  ...,
    moduleNameMapper: {
    '^src(.*)$': '<rootDir>/src/$1',
    // Related to this: https://jestjs.io/docs/28.x/upgrading-to-jest28#packagejson-exports
    uuid: require.resolve('uuid')
  },
  transform: {
    '\\.(ts|tsx)$': '<rootDir>/fix-istanbul-decorators.js'
  },
  ...
}

Hope it will help !

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