Pablissimo / SonarTsPlugin

SonarQube plugin for TypeScript files
MIT License
185 stars 105 forks source link

Sonar coverage for TS files do not match with my local results #99

Open parky128 opened 7 years ago

parky128 commented 7 years ago

Hi,

So I have this plugin configured and reading lcov files I am generating using istanbul along with istanbul-remap.

I am finding that for files I have 100% covered and appear as 100% covered in my local results, these files do not show as 100% by Sonar.

For example

image

This file is at 100% in my local html version of my coverage report from istanbul but shows as 99.1% in Sonar and I can see the opening line of the file is highlighted in yellow stating I havent covered all the conditions, yet none of the conditions in my actual source code are highlighted in Sonar, all apart from this opening line are highlighted green

image

image

This is happening across all my TS files, and so the overall coverage computed by Sonar falls well short of what I am seeing locally through istanbul.

Anyone come across this and can tell me what may be the problem?

Thanks

parky128 commented 7 years ago

I think I may have found out whats causing the differences I am seeing.

So we have gulp task to compile our TS in JS sources, but part of this task includes adding istanbul ignore comments around generated conditionals that the transpiler creates:

gulp.task('compile-typescript', ['clean-generated-scripts'], function() {
  var tsResult = tsProject
    .src()
    .pipe(sourcemaps.init())
    .pipe(tsProject());

  return tsResult.js.pipe(replace(/(}\)\()(.*\|\|.*;)/g, '$1/* istanbul ignore next */$2'))
    .pipe(replace(/(var __extends = \(this && this.__extends\))/g, '$1/* istanbul ignore next */'))
    .pipe(sourcemaps.write())
    .pipe(gulp.dest('./src'));
});

So using the file I have referenced in my original post, the corresponding js file contains these lines:

    })(/* istanbul ignore next */Controllers = InterACT.Controllers || (InterACT.Controllers = {}));
})(/* istanbul ignore next */InterACT || (InterACT = {}));

Which I m guessing are the 2 uncovered conditions the expanded sonar coverage results for the TS file in my original question could be related to?

With these ignore lines, the istanbul summary I get in my command line shows there are ignored branches:

============ Coverage summary 
Statements   : 95.65% ( 8316/8694 )
Branches     : 92.14% ( 2123/2304 ), 356 ignored
Functions    : 93.82% ( 2307/2459 )
Lines        : 95.44% ( 7499/7857 )
=============

I have removed the pipe() calls in the gulp task that performs the string replace for the istanbul ignore comments and I now see the same 2 uncovered conditions in my local reports so I think I am on to something here?!!

I am using istanbul-remap and am using the remapped coverage file (which of course references the TS files) for this sonar TS plugin so it should be honouring these ignore branches but doesnt seem to be. Am I missing something fundamental here?

Pablissimo commented 7 years ago

Is there any chance you can supply a minimal TS-plus-test-plus-gulp project that I can reproduce with? I've not really explored the impact of using istanbul-remap (my own projects use Chutzpah which natively outputs sourcemap-adjusted LCOV files).

I think what we might be hitting here is a limitation of my lifting of the LCOV parser from the JavaScript plugin. SonarQube will blindly trust, so far as I know, the LCOV parser's output in terms of finding out how many conditions exist on any given line and the coverage of them. I'm wondering whether we're hitting a case where there's a conditional in the JS that doesn't exist in the TS, but that the remap step isn't removing the branch bits.

The quick and dirty fix plugin-wise would be to ignore branch coverage information from the LCOV file, since unless the remapping back to TS-land can reliably tell what was a branch within the original source and what was a branch introduced by transpilation then the information's untrustworthy anyway.

Is it always the first couple lines in the file (i.e. whatever 'module MyModule' transpiles to) that cause the trouble?

parky128 commented 7 years ago

Thanks for the reply.

In the project we are working on, we actually have two applications, a server side nodejs application, and client side angularJS written in typescript.

So our gulp test task will perform the following steps:

  1. Run our karma\jasmine tests on the compiled angular TS files (.js sources)
  2. Run mocha\chai tests for our server side javascript code.
  3. Use gulp-remap on the client side report to generate an updated version of the report to refer to the remapped typescript files.
  4. Use gulp istanbul-combine to stitch together both the remapped coverage file and the original server side report files - this combined report gets output to html and also a combined lcov file.

I'm wondering whether we're hitting a case where there's a conditional in the JS that doesn't exist in the TS, but that the remap step isn't removing the branch bits

The combined html report we get at the end does not show the issue of missing conditionals, so that must mean the remap process is serving its purpose and correctly skipping the branches we want ignored using the istanbul ignore comments mention in my earlier posts?

Is it always the first couple lines in the file (i.e. whatever 'module MyModule' transpiles to) that cause the trouble?

Looking across the transpiled JS sources, in most cases its actually the last two lines, but I have found cases where its a few lines before the last lines of the files and its just a matter we are not yet following a strict style\layout guide with our TS sources.

Is there any chance you can supply a minimal TS-plus-test-plus-gulp project that I can reproduce with?

I will try and get something together that you can fork.

Am I better off trying out Chutzpah? I see there is a gulp version of the tool.

Pablissimo commented 7 years ago

Chutzpah is worth a go but to be honest I'd sooner the plugin just worked with your tooling given Istanbul is pretty common! I'll independently try to repro too, will at least give me some experience with the use case.

-------- Original message -------- From: Rob Parker notifications@github.com Date: 01/02/2017 22:11 (GMT+00:00) To: Pablissimo/SonarTsPlugin SonarTsPlugin@noreply.github.com Cc: Pablissimo turnacre@hotmail.com, Comment comment@noreply.github.com Subject: Re: [Pablissimo/SonarTsPlugin] Sonar coverage for TS files do not match with my local results (#99)

Thanks for the reply.

In the project we are working on, we actually have two applications, a server side nodejs application, and client side angularJS written in typescript.

So our gulp test task will perform the following steps:

  1. Run our karma\jasmine tests on the compiled angular TS files (.js sources)
  2. Run mocha\chai tests for our server side javascript code.
  3. Use gulp-remap on the client side report to generate an updated version of the report to refer to the remapped typescript files.
  4. Use gulp istanbul-combine to stitch together both the remapped coverage file and the original server side report files - this combined report gets output to html and also a combined lcov file.

I'm wondering whether we're hitting a case where there's a conditional in the JS that doesn't exist in the TS, but that the remap step isn't removing the branch bits

The combined html report we get at the end does not show the issue of missing conditionals, so that must mean the remap process is serving its purpose and correctly skipping the branches we want ignored using the istanbul ignore comments mention in my earlier posts?

Is it always the first couple lines in the file (i.e. whatever 'module MyModule' transpiles to) that cause the trouble?

Looking across the transpiled JS sources, in most cases its actually the last two lines, but I have found cases where its a few lines before the last lines of the files and its just a matter we are not yet following a strict style\layout guide with our TS sources.

Is there any chance you can supply a minimal TS-plus-test-plus-gulp project that I can reproduce with?

I will try and get something together that you can fork.

Am I better off trying out Chutzpah? I see there is a gulp version of the tool.

- You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/Pablissimo/SonarTsPlugin/issues/99#issuecomment-276799384, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABZvgBqB8CKgAq1Bi2t4Y6zbAESFJtxeks5rYQLygaJpZM4L0NmI.

parky128 commented 7 years ago

Ok thanks, I really appreciate it, I will try and get something together you can use to replicate but not sure how soon I can get it over.

What you said about their being a conditional present in the transpiled JS that is not in the source Typescript is definitely the thing to focus on here.

Let me know how you get on. Thanks

parky128 commented 7 years ago

@Pablissimo I just came across this - http://stackoverflow.com/questions/31746239/how-to-ignore-some-statements-in-javascript-code-coverage-when-using-lcov-and-is

These are the type of ignore comments we are adding to our transpiled JS files, although Sonar wouldnt be analysing the JS files since I am making sure these are excluded from analysis.

Anyway, thought I'd reference that SO question & answer

parky128 commented 7 years ago

I have taken a closer look at the produced remapped lcov file for the TS sources. Using the example non-smart-journeys-entry file in my earlier posts, if I search for that file section in the lcov file I can see it marks all the branches as being hit:

BRF:62
BRH:62

So its further confusing why Sonar thinks other wise.

I did come across this SO question - http://stackoverflow.com/questions/16988041/sonar-branch-coverage-on-class-declaration

I wondered if this could be a further clue, whilst this question is related to Java class declarations being analysed for branch coverage by the JaCoCo plugin, I wondered if this was a clue with the behaviour I am seeing since its always the namespace and export namespace lines in my TS files thats are marked as not covered by Sonar, e.g:

namespace Foo { //marked as not covered
  export namespace Bar { //marked as not covered
    export class MyClass { //marked as covered
Pablissimo commented 7 years ago

I'm starting to wonder if the real cause of this as a problem is that none of the three lines you have in your example even 'coverable' lines in a traditional sense - why would they be expected to contribute to coverage at all, positively or negatively?

In C# and Java for example, a line defining a class isn't (typically, depending on the coverage framework) considered coverable since it's not executable, just static. So in the following example, how many lines of code are coverable?

class DumbMaths {
  double(val: number) {
    return number * 2;
  }
}

At the minute, the plugin considers that to be 5 lines of code - of which 4 lines are 'covered' just by virtue of the file being part of your test harness - you don't have to have written a single test and you're at 80% coverage. Arguably that's only 1 line of code - the return that's doing the work.

Might do some deeper diving on how different coverage frameworks handle this stuff.

parky128 commented 7 years ago

Ok, thanks for giving this further thought. I have yet to put together a small sample for you to reproduce the behaviour I am seeing, I'll look to get something to you over the next few days. The Istanbul docs show how code coverage can be ignored here https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md

It should just work using the code comments I have on place as referenced in my earlier posts.

parky128 commented 7 years ago

@Pablissimo

Hello again, I am still no closer to understanding why Sonar is showing my files as not being 100% covered when they clearly are according to the lcov reports being generated.

Take another example here image

And an extract from my lcov for this particular file:

SF:src/client/app/features/settlement/transport-operator-services/transport-operator-services-list.controller.ts FN:1,(anonymous_1) FN:2,(anonymous_2) FN:3,(anonymous_3) FN:7,TransportOperatorServicesListController FN:43,(anonymous_5) FN:55,(anonymous_6) FN:71,(anonymous_7) FN:20,(anonymous_8) FN:21,(anonymous_9) FNF:9 FNH:9 FNDA:1,(anonymous_1) FNDA:1,(anonymous_2) FNDA:1,(anonymous_3) FNDA:8,TransportOperatorServicesListController FNDA:5,(anonymous_5) FNDA:2,(anonymous_6) FNDA:1,(anonymous_7) FNDA:8,(anonymous_8) FNDA:8,(anonymous_9) DA:1,2 DA:2,2 DA:3,9 DA:4,1 DA:7,8 DA:8,8 DA:9,8 DA:10,8 DA:20,1 DA:21,8 DA:22,8 DA:43,8 DA:44,5 DA:46,5 DA:47,4 DA:50,5 DA:51,4 DA:54,5 DA:55,5 DA:56,2 DA:57,2 DA:71,8 DA:72,1 DA:73,1 DA:75,1 DA:85,1 LF:26 LH:26 BRDA:46,1,0,4 BRDA:46,1,1,1 BRDA:50,2,0,4 BRDA:50,2,1,1 BRDA:55,3,0,4 BRDA:55,3,1,1 BRDA:2,4,0,1 BRDA:2,4,1,0 BRDA:1,5,0,1 BRDA:1,5,1,0 BRF:10 BRH:10 end_of_record

The last two lines show 10 branches found (BRF) and 10 hit (BRH).

I wondered if there was some thing the standard coverage plugin in Sonar could be doing to interpret these results differently.

Do you have any ideas? Thanks

Pablissimo commented 7 years ago

So I think the issue is still as I initially described - and you can see it in that last pair of BRDAs. Note: I'm no expert on LCOV, but here goes:

BRDA:1,5,0,1
BRDA:1,5,1,0

This is saying, so far as I understand it, that on line 1 there are two branches indexed 0 and 1.

Unsure why the BRF/BRH lines suggest all branches were hit, since they weren't according to the BRDAs. Also - notice that the second-to-last pair of BRDAs (referencing line 2) say the same thing - which is why you've got lines 1 and 2 with uncovered branches in SQ.

What's the first three lines of your TypeScript file, out of interest? I have an idea...

Pablissimo commented 7 years ago

Actually, scratch that if the following is analogous from your previous message:

namespace Foo { //marked as not covered
  export namespace Bar { //marked as not covered
    export class MyClass {

However - need to know what version of the TypeScript compiler you're running too if poss.

parky128 commented 7 years ago

Hi, yes all our TS files start as per that example, so the same opening 3 lines in each file.

Currently using gulp-typescript version 3.1.3

Pablissimo commented 7 years ago

Think I've got an answer on the why of all this, your assessment is correct but I think I can work through the stages now. I'm going to use this as the basis

namespace Foo {
    export class MyClass {
    }
}

Transpiles to:

var Foo;
(function (Foo) {
    var MyClass = (function () {
        function MyClass() {
        }
        return MyClass;
    }());
    Foo.MyClass = MyClass;
})(Foo || (Foo = {}));

When that gets compiled, we also get a source map generated - that lets us get back from a given line/column in the generated JavaScript to its corresponding line in the TypeScript. A source map for the above maps the first two lines and the last line of the JavaScript all to line 1 of the source TypeScript.

Trouble now though is that conditional on the last line - if this is the only class in that namespace, then we hit both sides of the condition. If there are multiple classes in the namespace then subsequent ones will only hit the first half of the condition.

If I'm right, you should find that you have one file per namespace that has 100% coverage, and all the others have slightly less than that. Wonder if you can confirm.

Regardless there's only two fixes I can think of:

Might be wrong on the above, but it seems to fit with the sorts of LCOV you're getting.

parky128 commented 7 years ago

If I'm right, you should find that you have one file per namespace that has 100% coverage, and all the others have slightly less than that. Wonder if you can confirm.

I have looked through all the files under each namespace in Sonar, and none are hitting 100%, each are just below.

Your first fix you have suggested sounds like the best approach, I wouldnt want branch coverage to get lost . Are you talking about making changes to this (your) plugin or needing to get the generic coverage plugin from Sonar changed via a PR?

Thanks for your time on this, much appreciated by the way!

Pablissimo commented 7 years ago

It's not a SonarQube issue, it's the plugin. My coverage sensor implementation is pretty dumb - if a line appears in the LCOV file and we haven't previously identified it as a comment line then we blindly tell SonarQube that we hit the line or not (depending on what's in the LCOV).

Just needs expanded to ignore not just comments but other non-coverable lines. However, that's getting hilariously close to being a partial parser of TypeScript unless I'm careful...

parky128 commented 7 years ago

Right ok, well if you fancy using me as guinea pig and want me to try out a patch release in our Sonar instance then be my guest :+1:

Pablissimo commented 7 years ago

Shall take you up on that! This isn't likely to be a short-term thing though - you may find you need to do some post-processing on your LCOV file to strip offending BRDAs to get you out of a hole for now.

parky128 commented 7 years ago

Ok interesting, I'll give that a try. Any thoughts on timescales for a patch from yourself? No doubt you are a busy guy!

Pablissimo commented 7 years ago

Not a scooby-doo I'm afraid, aside from having a job it's currently more a problem of it not being obvious how best to approach the problem! Have created #116 to track the work, and it does sound fun though my wife may disagree...

parky128 commented 7 years ago

@Pablissimo - we are using the istanbul-remap module and I found a bug they are tracking here

This is exactly the issue going on in my case, so I'll add some further evidence for them. They have marked as a bug anyway, and no doubt if they fix this the problem will go away for us.

Pablissimo commented 7 years ago

Ahha! Sweet, I think I need still to try to handle things better on our side too but the combination should be a treat if they figure out a fix. Though to be honest, the fix for them is the same as the fix for us - a partial parse of the source TypeScript file to filter the LCOV data when remapping.

vpassapera commented 7 years ago

Having this problem as well using angular cli. I got it to ignore interfaces, etc. But, since .spec files live in the same directory as normal files, it gets wonky:

sonar.projectKey=...
sonar.projectName=...
sonar.projectVersion=develop

sonar.language=ts
sonar.sourceEncoding=UTF-8
sonar.sources=src/app
sonar.tests=src/app
sonar.exclusions=**/node_modules/**,**/*.spec.ts,src/app/**/*.interface.ts,src/app/index.ts,src/app/layout/components/full-layout.component.ts,src/app/layout/components/breadcrumbs.component.ts,src/app/layout/directives/*.ts,src/app/layout/services/script.service.ts,src/app/account/components/login/*
sonar.test.inclusions=src/app/**/*.spec.ts
sonar.ts.tslint.outputPath=build/lint/issues.json
sonar.ts.coverage.lcovReportPath=build/coverage/lcov.info

However, slightly different situation when it comes to karma:

// Karma configuration file, see link for more information
// https://karma-runner.github.io/0.13/config/configuration-file.html

module.exports = function (config) {
  config.set({
    basePath: '',
    frameworks: ['jasmine', 'sinon', '@angular/cli'],
    captureTimeout: 60000,
    browserDisconnectTimeout: 10000,
    browserDisconnectTolerance: 3,
    browserNoActivityTimeout: 60000,
    plugins: [
      require('karma-jasmine'),
      require('karma-sinon'),
      require('karma-chrome-launcher'),
      require('karma-firefox-launcher'),
      require('karma-junit-reporter'),
      require('karma-jasmine-html-reporter'),
      require('karma-coverage-istanbul-reporter'),
      require('@angular/cli/plugins/karma')
    ],
    client: {
      clearContext: false // leave Jasmine Spec Runner output visible in browser
    },
    files: [
      { pattern: './src/test.ts', watched: false }
    ],
    preprocessors: {
      './src/test.ts': ['@angular/cli']
    },
    mime: {
      'text/x-typescript': ['ts', 'tsx']
    },
    coverageIstanbulReporter: {
      reports: ['html', 'lcovonly', 'text', 'text-summary'],
      dir: 'build/coverage',
      fixWebpackSourcePaths: true,
      thresholds: {
        emitWarning: false,
        each: {
          statements: 94,
          lines: 94,
          functions: 94
        }
      }
    },
    angularCli: {
      environment: 'dev'
    },
    reporters: config.angularCli && config.angularCli.codeCoverage
              ? ['junit', 'progress', 'coverage-istanbul']
              : ['progress', 'kjhtml'],
    junitReporter: {
      outputDir: 'build/coverage',
      outputFile: 'junit.xml',
      useBrowserName: false
    },
    port: 9876,
    colors: true,
    logLevel: config.LOG_INFO,
    autoWatch: true,
    browsers: ['ChromeHeadless', 'Firefox'],
    singleRun: true,
    customLaunchers: {
      ChromeHeadless: {
        base: 'Chrome',
        flags: [
          '--headless',
          '--disable-gpu',
          '--remote-debugging-port=9222'
        ]
      }
    }
  });
};

Angular CLI:

{
  "$schema": "./node_modules/@angular/cli/lib/config/schema.json",
  "project": {
    "version": "0.0.1",
    "name": "my-project"
  },
  "apps": [
    {
      "root": "src",
      "outDir": "dist",
      "assets": ["assets"],
      "index": "index.html",
      "main": "main.ts",
      "polyfills": "polyfills.ts",
      "test": "test.ts",
      "tsconfig": "tsconfig.app.json",
      "testTsconfig": "tsconfig.spec.json",
      "prefix": "app",
      "styles": [
        "../node_modules/ladda/css/ladda.scss",
        "../node_modules/dragula/dist/dragula.css",
        "../node_modules/font-awesome/css/font-awesome.css",
        "../node_modules/hint.css/hint.css",
        "assets-src/scss/style.scss"
      ],
      "scripts": [
        "../node_modules/identicon.js/pnglib.js",
        "../node_modules/identicon.js/identicon.js",
        "../node_modules/moment/min/moment.min.js",
        "../node_modules/moment-timezone/builds/moment-timezone-with-data.js",
        "../node_modules/chart.js/dist/Chart.bundle.min.js",
        "../node_modules/chart.js/dist/Chart.min.js",
        "../node_modules/fosjsroutingbundle/Resources/public/js/router.js"
      ],
      "environmentSource": "environments/environment.ts",
      "environments": {
        "dev": "environments/environment.ts",
        "ci": "environments/environment.ci.ts",
        "staging": "environments/environment.staging.ts",
        "prod": "environments/environment.prod.ts"
      }
    }
  ],
  "e2e": {
    "protractor": {
      "config": "./protractor.conf.js"
    }
  },
  "lint": [
    {
      "project": "src/tsconfig.app.json",
      "exclude": "**/node_modules/**/*"
    },
    {
      "project": "src/tsconfig.spec.json",
      "exclude": "**/node_modules/**/*"
    },
    {
      "project": "e2e/tsconfig.e2e.json",
      "exclude": "**/node_modules/**/*"
    }
  ],
  "test": {
    "codeCoverage": {
      "exclude": [
        "src/app/account/components/login/*",
        "src/app/layout/components/full-layout.component.ts",
        "src/app/layout/components/breadcrumbs.component.ts",
        "src/app/layout/directives/*.ts",
        "src/app/layout/services/script.service.ts"
      ]
    },
    "karma": {
      "config": "./karma.conf.js"
    }
  },
  "defaults": {
    "styleExt": "scss",
    "prefixInterfaces": false,
    "poll": 100
  }
}

Karma reports 100% test coverage (99.9x)

Sonar reports 93%.