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

TypeError: Cannot read properties of undefined (reading 'endCol') #6297

Open
6 tasks done
stramel opened this issue Aug 7, 2024 · 9 comments
Open
6 tasks done

TypeError: Cannot read properties of undefined (reading 'endCol') #6297

stramel opened this issue Aug 7, 2024 · 9 comments
Labels
feat: coverage Issues and PRs related to the coverage feature p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@stramel
Copy link

stramel commented Aug 7, 2024

Describe the bug

Appears to be a regression of a previous bug: #5639
Related PR for previous bug: #5642

When running coverage using @vitest/coverage-v8, with CJS files, it fails to properly report coverage.

As noted in the previous issue, adding a comment above the export makes the coverage work.

npm run cover

> cover
> vitest run --coverage


 RUN  v2.0.5 /Users/mstramel/PayPal/vitest-v8-coverage-repro
      Coverage enabled with v8

 ✓ test/basic.test.js (1)
   ✓ levels (1)
     ✓ should pass

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
TypeError: Cannot read properties of undefined (reading 'endCol')
 ❯ range.sliceRange node_modules/@vitest/coverage-v8/dist/provider.js:423:55
 ❯ CovSource.offsetToOriginalRelative node_modules/@vitest/coverage-v8/dist/provider.js:1584:20
 ❯ V8ToIstanbul._maybeRemapStartColEndCol node_modules/@vitest/coverage-v8/dist/provider.js:1987:92
 ❯ node_modules/@vitest/coverage-v8/dist/provider.js:1896:60
 ❯ node_modules/@vitest/coverage-v8/dist/provider.js:1894:20
 ❯ V8ToIstanbul.applyCoverage node_modules/@vitest/coverage-v8/dist/provider.js:1893:12
 ❯ node_modules/@vitest/coverage-v8/dist/provider.js:2609:21




 ✓ test/basic.test.js (1)
   ✓ levels (1)
     ✓ should pass
close timed out after 10000ms
Tests closed successfully but something prevents Vite server from exiting
You can try to identify the cause by enabling "hanging-process" reporter. See https://vitest.dev/config/#reporters

Reproduction

Reproduction on GitHub because StackBlitz doesn't support @vitest/coverage-v8.

Minimal Reproduction: https://github.com/stramel/vitest-v8-coverage-repro/tree/main

Steps:

  • Run npm run cover
  • Observe error

Expected Result: Coverage should finish correctly without error
Actual Result: Coverage doesn't finish and results in the error TypeError: Cannot read properties of undefined (reading 'endCol')

If you change src/basic.js to this

"use strict";

// FIX ISSUE

module.exports = {
  debug: 0,
  info: 1,
  warn: 2,
  error: 3,
  fatal: 4,
};

then run npm run cover it will work as expected.

System Info

`npx envinfo --system --npmPackages '{vitest,@vitest/*,vite,@vitejs/*}' --binaries --browsers`


  System:
    OS: macOS 14.5
    CPU: (12) arm64 Apple M2 Max
    Memory: 151.23 MB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.20.4 - ~/.volta/tools/image/node/18.20.4/bin/node
    Yarn: 1.22.19 - ~/.volta/bin/yarn
    npm: 10.8.2 - ~/.volta/tools/image/npm/10.8.2/bin/npm
  Browsers:
    Chrome: 127.0.6533.73
    Chrome Canary: 123.0.6270.0
    Edge: 127.0.2651.86
    Safari: 17.5
  npmPackages:
    @vitest/coverage-v8: latest => 2.0.5
    @vitest/ui: latest => 2.0.5
    vite: latest => 5.3.5
    vitest: latest => 2.0.5

Used Package Manager

npm

Validations

@AriPerkkio
Copy link
Member

What's this line? It's importing something that doesn't exist. Removing it makes the coverage work.

https://github.com/stramel/vitest-v8-coverage-repro/blob/0dea8002ec3583e2f244c8d5d10c9896b4ad1bff/test/basic.test.js#L2

@stramel
Copy link
Author

stramel commented Aug 8, 2024

What's this line? It's importing something that doesn't exist. Removing it makes the coverage work.

stramel/vitest-v8-coverage-repro@0dea800/test/basic.test.js#L2

@AriPerkkio It's importing the package itself. It's equivalent to importing ../src/index.js

AriPerkkio added a commit to AriPerkkio/vitest that referenced this issue Aug 8, 2024
@AriPerkkio AriPerkkio added feat: coverage Issues and PRs related to the coverage feature p2-edge-case Bug, but has workaround or limited in scope (priority) and removed pending triage labels Aug 8, 2024
@AriPerkkio
Copy link
Member

AriPerkkio commented Aug 8, 2024

Appears to be a regression of a previous bug: #5639

I don't think this is regression. Has this ever worked?

  1. Calling import levels from "../src/basic"; ends up loading basic through Vite, and it's executed by vite-node.
{
  "scriptId": "468",
  "url": "file:///Users/x/vitest/test/coverage-test/fixtures/src/cjs-package/basic.js",
  "functions": [
    {
      "functionName": "",
      "ranges": [
        {
          "startOffset": 0,
          "endOffset": 1031, // <-- Notice that this doesn't match file's actual content -> It's executed by vite-node that adds wrapper code around it.
          "count": 1
        }
      ],
      "isBlockCoverage": true
    }
  ]
}
  1. Calling import test from ".."; loads "src/index.js" from package.json's "main" which isn't intercepted by Vite. The index.js will then require('basic') which is again not intercepted by Vite.
{
  "scriptId": "469",
  "url": "file:///Users/x/vitest/test/coverage-test/fixtures/src/cjs-package/basic.js",
  "functions": [
    {
      "functionName": "",
      "ranges": [
        {
          "startOffset": 0,
          "endOffset": 95, // <-- Notice, no vite-node's wrapper used. Only the file itself was executed
          "count": 1
        }
      ],
      "isBlockCoverage": true
    }
  ]
}

I'm not sure if this can be fixed properly at all. Maybe we should finally just make the decision to completely ignore files that were not loaded via Vite.

You can fix this in your codebase by not relying on CommonJS when you are using Vite/Vitest.

@AriPerkkio
Copy link
Member

Also note that @vitest/coverage-istanbul might work out-of-the-box here. It does not include CommonJS files as it does the instrumentation using Vite plugins.

V8 coverage includes all files that Node executed so we don't really control that part.

@AriPerkkio
Copy link
Member

I think we could add a try-catch around here with warning logging when this kind of issues happen. It would then not fail whole report generation.

converter.applyCoverage(functions)

@stramel
Copy link
Author

stramel commented Aug 11, 2024

I don't think this is regression. Has this ever worked?

I was basing this on your comment in the PR for the issue where you said:

This is an edge case that can happen if a file was loaded outside Vitest, e.g. using require()

Perhaps its semi-related but not quite the same issue / not a regression. I still find it a bit strange that adding the comment fixes the issue of the import.

On this note:

I'm not sure if this can be fixed properly at all. Maybe we should finally just make the decision to completely ignore files that were not loaded via Vite.

You can fix this in your codebase by not relying on CommonJS when you are using Vite/Vitest.

We're working on migrating to ESM and away from CJS currently. We just finished migrating to Vitest from a slew of other test runners (mocha, tap, tape, jest, etc) and our next step is starting to convert to ESM. We were hoping that we would be able to get all our coverage working through the v8 provider before moving to that step though.

I think we could add a try-catch around here with warning logging when this kind of issues happen. It would then not fail whole report generation.

I do think this would be a good idea as it would be helpful for people in similar cases.

@AriPerkkio
Copy link
Member

I still find it a bit strange that adding the comment fixes the issue of the import.

It may seem that it works, but it's still causing incorrect coverage results.

When import levels from "../src/basic"; is ran, we execute the basic.js in node:vm with following code:

'use strict';async (__vite_ssr_import__,__vite_ssr_dynamic_import__,__vite_ssr_exports__,__vite_ssr_exportAll__,__vite_ssr_import_meta__,require,exports,module,__filename,__dirname)=>{{"use strict";

module.exports = {
  debug: 0,
  info: 1,
  warn: 2,
  error: 3,
  fatal: 4,
};

//# sourceMappingSource=vite-node
//# sourceMappingURL=data:application/json;charset=utf-8;base64,...

}}

And when require('basic') is ran, NodeJS runs this code as:

"use strict";

module.exports = {
  debug: 0,
  info: 1,
  warn: 2,
  error: 3,
  fatal: 4,
};

Now when we get coverage results for this, they look like shown in #6297 (comment). In order to properly remap the coverage results back to source code, we need to apply offset in the beginning to get rid of the 'use strict';async (__vite_ssr_import__ ... wrapper code that vite-node adds. However now we are also applying it to the second case that wasn't run with this wrapper.

When the wrapper code is incorrectly assumed to be in the V8 coverage result, we simply read out-of-bounds of the original source file. When you add new comment (or any other code) there, it luckily adds enough characters so that this out-of-bounds read doesn't happen. But it's still incorrectly mapped so results are off.

I did some testing whether we could actually detect from the V8 results whether vite-node wrapper was there: 9f571ee .
It could work fine but I'm not sure if we want to apply this for every single user. But even if we end up adding this logic later, I still think we should proceed with #6318 as that error might happen in other cases as well.

@stramel
Copy link
Author

stramel commented Aug 11, 2024

Ah, that makes a lot more sense! Thank you for the explanation! I agree that proceeding with the warning and not blocking coverage would be the best route

@stramel
Copy link
Author

stramel commented Aug 13, 2024

Just confirmed that the fix worked by testing 2.1.0-beta.5. Thank you for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: coverage Issues and PRs related to the coverage feature p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

No branches or pull requests

2 participants