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

coverage results for typescript are non-deterministic #1148

Closed
1 task done
Kosta-Github opened this issue Jul 17, 2019 · 19 comments
Closed
1 task done

coverage results for typescript are non-deterministic #1148

Kosta-Github opened this issue Jul 17, 2019 · 19 comments

Comments

@Kosta-Github
Copy link

Kosta-Github commented Jul 17, 2019

Link to bug demonstration repository

demo repo: https://github.com/Kosta-Github/ts-nyc-issue

the typescript setup described in nyc-config-typescript has been followed

Expected Behavior

running one of the 2 provided test scripts several times should always result in the same (correct) output/result

Observed Behavior

Sometimes the result looks like this and the generated HTML coverage report looks OK:

-------------|----------|----------|----------|----------|-------------------|
File         |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------|----------|----------|----------|----------|-------------------|
All files    |       80 |       50 |    66.67 |       80 |                   |
 func.ts     |    83.33 |       50 |      100 |    83.33 |                 8 |
 nested_1.ts |      100 |      100 |      100 |      100 |                   |
 nested_2.ts |       50 |      100 |        0 |       50 |                 2 |
-------------|----------|----------|----------|----------|-------------------|

=============================== Coverage summary ===============================
Statements   : 80% ( 8/10 )
Branches     : 50% ( 1/2 )
Functions    : 66.67% ( 2/3 )
Lines        : 80% ( 8/10 )
================================================================================

And sometimes the result looks like this:

-------------|----------|----------|----------|----------|-------------------|
File         |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-------------|----------|----------|----------|----------|-------------------|
All files    |       40 |       50 |    66.67 |       80 |                   |
 func.ts     |       50 |       50 |      100 |      100 |                 5 |
 nested_1.ts |       50 |      100 |      100 |      100 |                   |
 nested_2.ts |        0 |      100 |        0 |        0 |                 2 |
-------------|----------|----------|----------|----------|-------------------|

=============================== Coverage summary ===============================
Statements   : 40% ( 4/10 )
Branches     : 50% ( 1/2 )
Functions    : 66.67% ( 2/3 )
Lines        : 80% ( 4/5 )
================================================================================

And in this case the generated HTML files contain this error message:

Cannot read property 'start' of undefined
TypeError: Cannot read property 'start' of undefined
    at Object.keys.forEach.stName (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-reports/lib/html/annotator.js:53:31)
    at Array.forEach ()
    at annotateStatements (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-reports/lib/html/annotator.js:49:33)
    at Object.annotateSourceCode (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-reports/lib/html/annotator.js:239:9)
    at HtmlReport.onDetail (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-reports/lib/html/index.js:265:27)
    at LcovReport.(anonymous function) [as onDetail] (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-reports/lib/lcov/index.js:23:23)
    at Visitor.(anonymous function) [as onDetail] (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-lib-report/lib/tree.js:34:30)
    at ReportNode.Node.visit (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-lib-report/lib/tree.js:114:17)
    at getChildren.forEach.child (/Users/kosta/devel/misc/ts-nyc-issue/node_modules/istanbul-lib-report/lib/tree.js:118:15)
    at Array.forEach ()

Troubleshooting steps

  • still occurring when I put cache: false in my nyc config

Environment Information

$ npx envinfo@latest --preset nyc

  System:
    OS: macOS 10.14.5
    CPU: (4) x64 Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz
    Memory: 1.20 GB / 16.00 GB
  Binaries:
    Node: 10.16.0 - /usr/local/opt/node@10/bin/node
    Yarn: 1.17.3 - /usr/local/bin/yarn
    npm: 6.10.1 - /usr/local/opt/node@10/bin/npm
  npmPackages:
    nyc: ^14.1.1 => 14.1.1 
    ts-node: ^8.3.0 => 8.3.0 
    typescript: ^3.5.3 => 3.5.3 
@coreyfarrell
Copy link
Member

I believe to use "all": true you need to configure the require options on nyc instead of mocha. TBH I have not downloaded your repository I'm busy currently but this looks like something I've seen with babel transpilation.

CC @JaKXz my knowledge of TS is very limited but I think this might be an issue where the instrumentation for nyc --all is not being processed by ts-node/register where the instrumentation for files required by actual tests is being processed by ts-node/register, causing a mismatch.

@Kosta-Github
Copy link
Author

mh, actually looking into the config of nyc-config-typescript I think this is close to useless:

  • it doesn't do the require for ts-node and source-map-support
  • actually, all it effectively does is setting cache to false

IMHO, it would be better and way easier to just replace the documentation about adding this

{
    "extends": "@istanbuljs/nyc-config-typescript",
    ...
}

by describing to inject the following into the .nycrc.json instead:

{
    "require": [
        "ts-node/register",
        "source-map-support/register"
    ],
    "cache": false,
    "extension": [
        ".js",
        ".ts"
    ],
    ...
}

Adding the require option as described above seems to solve the issue about giving non-deterministic results.
However, it is still not clear to me, why it sometimes works and sometimes not without the require option defined in the nyc config?

@coreyfarrell
Copy link
Member

Each process records coverage to .nyc_output/${uuid}.json. I think what's happening is sometimes the coverage data generated by "all": true fails to merge properly with the coverage data generated by the tests. I suspect the merge fails when depending on the order of the all vs test coverage data is found.

Regarding the usefulness of @istanbuljs/nyc-config-typescript the current releases of nyc and test-exclude do not exclude Typescript test files by default. nyc@15 will ignore TypeScript tests and the extension list will include '.ts' by default, so for nyc@15 that preset will likely be unnecessary. It does seem like the documentation for this needs to be corrected to suggest configuring requires to nyc instead of mocha.

coreyfarrell added a commit to coreyfarrell/nyc that referenced this issue Aug 1, 2019
Sometimes the coverage data produced by `nyc --all` is incompatible with
the coverage data produced by actual test runs.  This is generally due
to configuration error but results in inconsistent coverage reports or
in some cases causes `nyc report` to crash.  The workaround is
implemented in istanbul-lib-coverage to drop coverage data associated
with `nyc --all` when coverage data from a test run is found.  This
commit tags the coverage data when appropriate so the coverage merge
logic knows what to do.

Fixes istanbuljs#1113, istanbuljs#1124, istanbuljs#1148
coreyfarrell added a commit that referenced this issue Aug 2, 2019
…#1155)

* fix: Drop coverage produced by `nyc --all` for files that were tested

Sometimes the coverage data produced by `nyc --all` is incompatible with
the coverage data produced by actual test runs.  This is generally due
to configuration error but results in inconsistent coverage reports or
in some cases causes `nyc report` to crash.  The workaround is
implemented in istanbul-lib-coverage to drop coverage data associated
with `nyc --all` when coverage data from a test run is found.  This
commit tags the coverage data when appropriate so the coverage merge
logic knows what to do.

Fixes #1113, #1124, #1148
@Kosta-Github
Copy link
Author

Kosta-Github commented Aug 5, 2019

Actually, the issues are coming back for more complex projects. Sadly, I was not able to create a small reproducible sample yet.

But as you can see in the attached screenshot below I am getting weird reports even in case of requiring ts-node/register and source-map-support/register from the nyc config...

image

And this is also independent from all: true vs. all: false setting...

@jednano
Copy link

jednano commented Sep 27, 2019

Same exact issue here, but I'm using AVA, not Mocha. Also, my project is open source. It only has one source file with one function and one test against it, so it's a perfect candidate to reproduce the issue on your end. All you have to do is clone jedmao/node-starter and remove the following from the nyc config in package.json:

    "require": [
      "ts-node/register",
      "source-map-support/register"
    ]

Run npm run cover and you'll see that 50% of the time the test coverage reports that only 50% of the statements are covered.

@coreyfarrell
Copy link
Member

@jedmao currently it is not optional to include these require statements on nyc configuration when you are using all: true. Otherwise you end up with incompatible coverage between the product of all: true and the actual test runs, and it randomly finds the all coverage OR the test coverage depending on the randomized identification of each coverage file per run. This is fixed by #1155 and a related patch to the istanbuljs libraries. This fix will be released as part of nyc@15. That said I do recommend always setting the correct configuration for the nyc.require option even after this is fixed in nyc@15.

@jednano
Copy link

jednano commented Sep 28, 2019

Just to be clear, are you saying I should keep my configuration the same as it is now and just update nyc to the latest version when it becomes available?

@coreyfarrell
Copy link
Member

Yes

@moribellamy
Copy link

Thanks for filing this bug.

I have an app, https://github.com/moribellamy/graytabby. I have the flakiness in 9817052f36cda3862b0bcc001a2e45d0b8aab9f3, and something about one commit later at 4abb5dac9d6ca0e42b1f15ff7298fed9f097ef88 causes it to stop:

https://github.com/moribellamy/graytabby/compare/9817052f36cda3862b0bcc001a2e45d0b8aab9f3..4abb5dac9d6ca0e42b1f15ff7298fed9f097ef88

you can check for the flakiness with this command. the more correct looking coverage has 11,22 as a substring.

npx nyc mocha --opts tests/unit.opts | grep 11,21

on the flaky commit, i can get one flake in 4, approximately. on the next commit i haven't gotten a flake in 20 or so coverage runs.

@Kosta-Github
Copy link
Author

@moribellamy so, do you think that --require ts-node/register/transpile-only fixes the issue?

@akoidan
Copy link

akoidan commented Nov 17, 2019

If you need to reproduce the issue:

run this multiple times, output of the grep would differ

@coreyfarrell
Copy link
Member

Everyone experiencing this issue please test with nyc@next. If you use babel-plugin-istanbul please be sure to also update that to the next version as well. As far as I know this issue is fixed in the next version of nyc.

@akoidan
Copy link

akoidan commented Nov 18, 2019

@coreyfarrell well, with @next the issue seems to be gone from nyc mocha with ts-node, but when I merge 2 outputs from cypress and mocha and then do a report I'm still getting the same issue:

At least it's always determenistic now, I always get:

TypeError: Cannot read property 'start' of undefined

Note that if you check

  • ./nyc/coverage_cypress/src/utils/xhr.ts.html
  • ./nyc/coverage_unit/xhr.ts.html

they are both ok. And it's werid that the format of output html is different

@coreyfarrell
Copy link
Member

@akoidan I've reopened #1226, lets discuss your issue there.

@stale
Copy link

stale bot commented Dec 4, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Dec 4, 2020
@akoidan
Copy link

akoidan commented Dec 5, 2020

The bug above still persitst

@stale stale bot removed the stale label Dec 5, 2020
@coreyfarrell
Copy link
Member

@akoidan this is not your issue, you are using cypress which is why I reopened #1226 for you. I understand you're frustrated but please consider this issue unrelated to your problem.

@Kosta-Github I have not heard from you on this issue in a while, has nyc 15 resolved your issue?

@Kosta-Github
Copy link
Author

@coreyfarrell looks like it is working for me right now with 15.x 👍

@coreyfarrell
Copy link
Member

@Kosta-Github that's great, thanks for the update!

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

No branches or pull requests

5 participants