weixin_39543758
weixin_39543758
2021-01-11 13:42

we should still run transpilers on source files included via --all

could you give this branch a shot and let me know if it does the trick for you?

Reviewers:

该提问来源于开源项目:istanbuljs/nyc

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

17条回答

  • weixin_39543758 weixin_39543758 4月前

    could you give the next tag of nyc a spin and let me know if things are fixed for you:

    npm i nyc

    点赞 评论 复制链接分享
  • weixin_39710660 weixin_39710660 4月前

    Not there yet. Now I got a different error:

    
    /react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:542
            throw new TypeError('Column must be greater than or equal to 0, got '
                  ^
    TypeError: Column must be greater than or equal to 0, got -2
        at SourceMapConsumer_findMapping [as _findMapping] (/react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:542:15)
        at SourceMapConsumer_originalPositionFor [as originalPositionFor] (/react-starterkit/node_modules/nyc/node_modules/source-map/lib/source-map-consumer.js:603:24)
        at mapLocation (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:33:25)
        at /react-starterkit/node_modules/nyc/lib/source-map-cache.js:93:18
        at Array.forEach (native)
        at SourceMapCache._rewriteStatements (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:92:40)
        at /react-starterkit/node_modules/nyc/lib/source-map-cache.js:23:11
        at Array.forEach (native)
        at SourceMapCache.applySourceMaps (/react-starterkit/node_modules/nyc/lib/source-map-cache.js:15:23)
        at NYC.writeCoverageFile (/react-starterkit/node_modules/nyc/index.js:296:25)
    

    This is the project I'm testing it on: https://github.com/albertogasparin/react-starterkit/tree/nyc-coverage

    点赞 评论 复制链接分享
  • weixin_39710660 weixin_39710660 4月前

    By looking at the source code, I've also tried adding --cache. However it looks like the option gets ignored, as in NYC.prototype.writeCoverageFile this.enableCache is always false when --all is also defined.

    点赞 评论 复制链接分享
  • weixin_39877581 weixin_39877581 4月前

    nyc works for me on https://github.com/jwhitfieldseed/prequel. Thanks!

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    I've reproduced 's problem (don't forget to install nyc in the checkout anybody else wants to try). Looks like the coverage report for the ./api/todos/index.js file has an illegal statement location:

    
    statementMap:
       { '1': { start: { line: 1, column: -2 }, end: { line: 1, column: 60 } },
    

    Seems like an Istanbul bug to me so perhaps you could report it upstream?.

    Meanwhile I did some hacks to normalize the positions, see this branch: https://github.com/novemberborn/nyc/tree/clamp-positions.

    do you think this is something we should fix on our end? I can turn that branch into a PR with some tests.

    点赞 评论 复制链接分享
  • weixin_39710660 weixin_39710660 4月前

    Thanks for pointing out the statementMap! Tried removing "retainLines": true in .babelrc and it worked! I'm wondering if it should be reported to istambul or if it is a problem on source map creation.

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    Applying the source map to the report that was created by Istanbul is what fails. retainLines would create some rather long lines I suppose, maybe that's tripping up Istanbul. I don't quite know enough about the Istanbul internals to reason about it further though.

    点赞 评论 复制链接分享
  • weixin_39543758 weixin_39543758 4月前

    great debugging; I would definitely be interested in patching this on our end, but I think we should also open an issue with Istanbul, and be prepared to pull our code out (mind doing this).

    want to open a pull request with your patch, and we can see if it fixes 's problem?

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    done the PR now (#150).

    I've tried reproducing the issue with 's code by transpiling the api/todos/index.js file, then covering it (like build-self-coverage does for NYC) and writing the report, but the positions are valid. That's awfully generic to report upstream :disappointed:

    点赞 评论 复制链接分享
  • weixin_39543758 weixin_39543758 4月前

    give this a try:

    npm i nyc

    If things are looking good, I'd like to promote next to latest tomorrow.

    点赞 评论 复制链接分享
  • weixin_39710660 weixin_39710660 4月前

    Tested nyc with both retainLines flag on and off, and both reports are generated as expected. Thanks a lot guys for fixing it so quickly :+1:

    On a side note, it is interesting that, given the same source, I get two slightly different reports:

     bash
    All files  | 42.03 | 36.21 | 30.19 | 41.61  # retainLines: true
    All files  | 43.55 | 42.59 | 26.83 | 42.37  # retainLines: false
    

    But I assume this is expected as the instrumented code is different.

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    On a side note, it is interesting that, given the same source, I get two slightly different reports:

    All files | 42.03 | 36.21 | 30.19 | 41.61 # retainLines: true All files | 43.55 | 42.59 | 26.83 | 42.37 # retainLines: false But I assume this is expected as the instrumented code is different.

    Given the illegal coverage positions returned by Istanbul we may not be able to map coverage to the correct source lines, hence the difference. But coverage is pretty far from 100% it seems, so relatively speaking it's not much of a concern. Would be interesting to see if the underlying error stops you from hitting 100%.

    点赞 评论 复制链接分享
  • weixin_39710660 weixin_39710660 4月前

    Well, it looks like that by approaching 100% coverage the difference disappears. Files with 100% coverage are 100% regardless of the flag, so it is a non issue unless you care about 81% vs 82% :wink:

    点赞 评论 复制链接分享
  • weixin_39782832 weixin_39782832 4月前

    regarding retainLines, FWIW, I'm getting a fairly significant discrepancy:

    
    All files | 72.7  | 51.94 | 49.83 | 95.31 |  # retainLines: true
    All files | 96.13 | 75.61 | 83.62 | 96.13 |  # retainLines: false
    

    Don't know if this is really worth pursuing anyway; I had it on to assist WebStorm with debugging.

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    I wonder if the source map is less accurate when retainLines is on, or maybe it surfaces issues when we map the coverage report. Seems like an edge case either way.

    点赞 评论 复制链接分享
  • weixin_39877581 weixin_39877581 4月前

    That works in my repro example and a real project. Thanks! :clap:

    点赞 评论 复制链接分享
  • weixin_39556064 weixin_39556064 4月前

    Looks good. Only thing I can think of is maybe to gitignore ./test/fixtures/needs-transpile.js in case a test crashes and it's left behind.

    点赞 评论 复制链接分享

相关推荐