Incorrect "if path not taken"

[Ketec]

private subscriberHandler(res: Response, subject: Subscriber<any>, url: string = "") {

The coverage shows the I "if path not taken" in front of url: string = "" Which is not a if block at all, but rather default value assignment.

[nozer]

I have the another incorrect "if path not taken" issue:

I checked and those paths are taken during tests yet the report says otherwise. I am using nyc which uses istanbul as dependency with following versions:

    "istanbul-lib-coverage": "^1.0.2",
    "istanbul-lib-hook": "^1.0.5",
    "istanbul-lib-instrument": "^1.7.0",
    "istanbul-lib-report": "^1.0.0",
    "istanbul-lib-source-maps": "^1.1.1",

Here is the full code

import { IOpAttributes } from './IOpAttributes';
import { ListType, AlignType, DirectionType, ScriptType } from './value-types';
import './extensions/String';

class OpAttributeSanitizer {

    static sanitize(dirtyAttrs: IOpAttributes): IOpAttributes {

        var cleanAttrs: any = {};

        if (!dirtyAttrs || typeof dirtyAttrs !== 'object') {
            return cleanAttrs;
        }

        let {
            font, size, link, script, list, header, align, direction, indent
        } = dirtyAttrs;

        ['bold', 'italic', 'underline', 'strike', 'code', 'blockquote', 'code-block']
            .forEach(function (prop: string) {
                cleanAttrs[prop] = !!(<any>dirtyAttrs)[prop];
            });

        ['background', 'color'].forEach(function (prop: string) {
            var val = (<any>dirtyAttrs)[prop];
            if (val && OpAttributeSanitizer.IsValidHexColor(val + '')) {
                cleanAttrs[prop] = val;
            }
        });

        if (font && OpAttributeSanitizer.IsValidFontName(font + '')) {
            cleanAttrs.font = font;
        }

        if (size && OpAttributeSanitizer.IsValidSize(size + '')) {
            cleanAttrs.size = size;
        }

        if (link) {
            cleanAttrs.link = (link + '')._scrubUrl();
        }

        if (script === ScriptType.Sub || ScriptType.Super === script) {
            cleanAttrs.script = script;
        }

        if (list === ListType.Bullet || list === ListType.Ordered) {
            cleanAttrs.list = list;
        }

        if (header && parseInt(header + '', 10) > 0) {
            cleanAttrs.header = Math.min(parseInt(header + '', 10), 6);
        }

        if (align === AlignType.Center || align === AlignType.Right) {
            cleanAttrs.align = align;
        }

        if (direction === DirectionType.Rtl) {
            cleanAttrs.direction = direction;
        }

        if (indent && parseInt(indent + '', 10) > 0) {
            cleanAttrs.indent = Math.min(parseInt(indent + '', 10), 30);
        }

        return <IOpAttributes>cleanAttrs;
    }

    static IsValidHexColor(colorStr: string) {
        return !!colorStr.match(/^#([0-9A-F]{6}|[0-9A-F]{3})$/i);
    }

    static IsValidFontName(fontName: string) {
        return !!fontName.match(/^[a-z\s0-9\- ]{1,30}$/i)
    }

    static IsValidSize(size: string) {
        return !!size.match(/^[a-z\-]{1,20}$/i)
    }
}

export { OpAttributeSanitizer }

[nozer]

@Ketec You can fix yours if you make a call to that function that passes the url argument as a parameter. Coverage report basically says that you did not call that function by giving that parameter explicitly.

[Vashnak]

I have the same issue than you @nozer, did you find a work around ?

[nozer]

@Vashnak It was completely my fault. I was using TypeScript and my test files had .ts extension. For one specific file that tested the aforementioned method, it had .js extension instead of .ts extension and so it was not picked up by test runner (it was configured to pick up .ts files). So, in this case, what I can say is that make sure your test file is being picked up and run by your test environment.

[kailashyogeshwar85]

@nozer I am facing the same issue for using enum in ts file but it shows error if path not taken, i have provided enough test cases for executing that line but still same issue.

[nozer]

I think your enum declarations are wrong. Look here https://www.typescriptlang.org/docs/handbook/enums.html

[kailashyogeshwar85]

@nozer It is right else compiler would throw an error

[nozer]

I think you should do it like this:

Change enum to object (since you are hiding this enum inside a function and not available globally, why does it have to be enum?):

let levelCodesValues = {
   10: 'trace',
   ...
   50: 'error'
}

Then, in your test, make calls to getLevelValue for each code:

getLevelValue(10);
getLevelValue(20);
...
getLevelValue(50);

In your code, you are trying to look up enum values incorrectly. levelCode must be one of 'trace', 'debug', etc and not a number.

[kailashyogeshwar85]

Here it is I changed enum to globally accessible in class

export enum LevelCodes {
  DEBUG = 'DEBUG',
  INFO = 'INFO',
  WARN = 'WARN',
  ERROR = 'ERROR'
}
class MyClass {
  ...
  public getLevelValue(levelCode: number): any {
    switch (levelCode) {
      case 20:
        return LevelCodes.DEBUG;
      case 30:
        return LevelCodes.INFO;
      case 40:
        return LevelCodes.WARN;
      case 50:
        return LevelCodes.ERROR;
      default:
        return undefined;
    }
  }
}
// test.ts
it('should test logger error level', (done) => {
        logger.error('test message');
        let log = readFileSync('logs/engine-b75619ad4d.log', 'utf8');
        log = JSON.parse(log);
        expect(log).toBeDefined();
        expect(log).toHaveProperty('level', 'ERROR');
        expect(log).toHaveProperty('message', 'test message');
        expect(log).toHaveProperty('name', 'engine');
        done();
 });

It is something really weird. :worried:

[nozer]

I don't see how that test.js file is testing the class / method in question.

In your test file, maybe something like this:

it('blah blah', (done) => {
let m = new MyClass();
expect(m.getLevelValue(20)).toBeEqual(LevelCodes.DEBUG);
...
expect(m.getLevelValue(50)).toBeEqual(LevelCodes.ERROR);
expect(m.getLevelValue(999)).toBeUndefined();
done();
});

You need to call your method multiple times with different codes so that all branches in the method body are hit.

And you don't really need that done param in callback and subsequent done() call since your test code is sync and not waiting for some promise etc to be done. I just put there not to confuse you further.

[kailashyogeshwar85]

Here is the full unit test code

import {PinoAdapter, LevelCodes} from '../src/core/logger/PinoAdapter';
import {existsSync, truncateSync, readFileSync, rmdirSync, emptyDirSync} from 'fs-extra';

const logOptions: Resources.LoggerOptions = {
    logpath: 'logs'
};

const meta: Resources.EngineInfoData = {
    version: '1',
    engine_id: 'b75619ad4d'
};

describe('#PinoAdapter Module', () => {
    let logger;

    it('should create the instance of logger', () => {
        emptyDirSync('logs');
        rmdirSync('logs');
        logger = new PinoAdapter('/__test__/pino.test', logOptions, meta);
        expect(logger).toBeInstanceOf(PinoAdapter);
    });

    it('should create logs directory if not exist', () => {
        expect(existsSync('logs')).toBeTruthy();
    });

    it('should create log file with engine_id in prefix name', () => {
        expect(existsSync('logs/engine-b75619ad4d.log')).toBeTruthy();
    });

    it('should test logger info level', (done) => {
        truncateSync('logs/engine-b75619ad4d.log', 0);
        logger.info('test info message');

        setTimeout(() => {
            let log = readFileSync('logs/engine-b75619ad4d.log', 'utf8');
            log = JSON.parse(log);
            expect(log).toBeDefined();
            expect(log).toHaveProperty('level', LevelCodes.INFO);
            expect(log).toHaveProperty('message', 'test info message');
            expect(log).toHaveProperty('name', 'engine');
            done();
        }, 2000);
    });

    it('should test logger debug level', (done) => {
        truncateSync('logs/engine-b75619ad4d.log', 0);
        logger.debug('test debug message');
        setTimeout(() => {
            let log = readFileSync('logs/engine-b75619ad4d.log', 'utf8');
            log = JSON.parse(log);
            expect(log).toBeDefined();
            expect(log).toHaveProperty('level', LevelCodes.DEBUG);
            expect(log).toHaveProperty('message', 'test debug message');
            expect(log).toHaveProperty('name', 'engine');
            done();
        }, 1000);
    });

    it('should test logger error level', (done) => {
        truncateSync('logs/engine-b75619ad4d.log', 0);
        logger.error('test message');
        setTimeout(() => {
            let log = readFileSync('logs/engine-b75619ad4d.log', 'utf8');
            log = JSON.parse(log);
            expect(log).toBeDefined();
            expect(log).toHaveProperty('level', LevelCodes.ERROR);
            expect(log).toHaveProperty('message', 'test message');
            expect(log).toHaveProperty('name', 'engine');
            done();
        }, 1000);
    });

    it('should test logger warn level', (done) => {
        truncateSync('logs/engine-b75619ad4d.log', 0);
        logger.warn('test message');
        setTimeout(() => {
            let log = readFileSync('logs/engine-b75619ad4d.log', 'utf8');

            log = JSON.parse(log);
            expect(log).toBeDefined();
            expect(log).toHaveProperty('level', LevelCodes.WARN);
            expect(log).toHaveProperty('message', 'test message');
            expect(log).toHaveProperty('name', 'engine');
            done();
        }, 1000);
    });

    it('should get level code strings', () => {
        expect(logger.getLevelValue(20)).toBe(LevelCodes.DEBUG);
        expect(logger.getLevelValue(30)).toBe(LevelCodes.INFO);
        expect(logger.getLevelValue(40)).toBe(LevelCodes.WARN);
        expect(logger.getLevelValue(50)).toBe(LevelCodes.ERROR);
        expect(logger.getLevelValue(30)).not.toBeFalsy();
    });

    it('should return undefined if level code does not match', () => {
        expect(logger.getLevelValue(80)).toBeUndefined();
        expect(logger.getLevelValue(undefined)).toBeFalsy();
        expect(logger.getLevelValue(null)).toBeFalsy();
    });

    it('should create take default log path if not specified', () => {
        emptyDirSync('logs');
        rmdirSync('logs');
        const options: Resources.LoggerOptions = { };
        const l2 = new PinoAdapter('/__test__/pino.test', options, meta);
        expect(l2).toBeDefined();
        expect(existsSync('logs')).toBeTruthy();
    });
});

[nozer]

Looking at this expect(logger.getLevelValue(20)).toBe(LevelCodes.DEBUG);, I don't use the expect library but isn't .toBe() used to for objects? You should try .toEqual() to compare primitive types like strings, numbers, booleans, etc.

[kailashyogeshwar85]

toBe can be used for checking object equality as well as primitive types. Nevermind will ignore it for now.

[JohnArrowwood]

I'm seeing this as well when I have an if statement that triggers a throw, and then test to make sure that the error is thrown.

The code:

const IDENTIFIER = /^[_a-z][_a-z0-9]*$/i;
// ...
export class CombinatorialEval {
// ...
  dimension( name: string, values: Array<string>, valid?: Expression ) {
    if( name === null ||
        name === undefined ||
        name === '' ||
        ! IDENTIFIER.test( name ) )
          throw new Error( "Name must be a valid identifier" );

I have test cases for each branch:

describe( "Error Handling inside CombinatorialEval.dimension()",  function() {
    let name = 'foo';
    let values = ['A','B','C'];
    let validate = new CallbackCounter();
    context( "name", function() {
        it( "should throw an error if name is null", function() {
            expect( () => new CombinatorialEval().dimension(null,values) ).to.throw;
        });
        it( "should throw an error if name is undefined", function() {
            expect( () => new CombinatorialEval().dimension(undefined,values) ).to.throw;
        });
        it( "should throw an error if name is an empty string", function() {
            expect( () => new CombinatorialEval().dimension('',values) ).to.throw;
        });
        it( "should throw an error if name is not a valid identifier", function() {
            expect( () => new CombinatorialEval().dimension('foo:bar',values) ).to.throw;
        });
    });

And they all pass:

  Error Handling inside CombinatorialEval.dimension()
    name
      ✓ should throw an error if name is null
      ✓ should throw an error if name is undefined
      ✓ should throw an error if name is an empty string
      ✓ should throw an error if name is not a valid identifier

But the coverage report still shows that the "if path not taken".

I tried not using Chai's expect(...).to.throw, like this:

    it( "should throw an error if name is null", function() {
        let thrown = false;
        try {
            new CombinatorialEval().dimension(null,values);
        } 
        catch (e) {
            thrown = true;
        }
        expect( thrown ).to.be.true;
        // expect( () => new CombinatorialEval().dimension(null,values) ).to.throw;
    });

This marks the If block as executed. So there is something about the way Chai is implementing expect(...).to.throw that is interfering with instrumentation.

[nozer]

You should have another it call where the name is one that is not null, undefined, empty string, and a valid identifier. Basically, all conditions in if must be false.

[artworkad]

Same happening to me

Even in the comment it says: else path not taken 😄

Similar issue: https://github.com/gotwarlost/istanbul/issues/735

[RPbarfield]

Seeing this same issue in my Istanbul code coverage.

The percentage of code coverage doesn't see to be affected, but the placements of uncovered code is completely off. Random (blacked out) IFs and ELSEs scattered throughout the page also verify something has gone sideways.

[Ratikant01]

I am having same issue!! Any update??

[Ratikant01]

[Ratikant01]

Not sure if this repo is managed anymore. Can someone please update if there is any alternative tool for code coverage to use with Angular 5?

[brian428]

I'm also seeing the same thing. The highlighted sections are totally wrong. Seems like something must be up either with Istanbul, or the source maps, or both?

[Ratikant01]

I can confirm the issue is with -sourcemap=false!! Mine it solved after removing this flag in Angular 5 app.

[lend-a-tailor]

The issue is still there whether I use --sourceMap=true or --sourceMap=false in angular 6.0.0.