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

Is the code coverage markup working properly? #181

Open
seanpoulter opened this issue Dec 5, 2017 · 11 comments
Open

Is the code coverage markup working properly? #181

seanpoulter opened this issue Dec 5, 2017 · 11 comments

Comments

@seanpoulter
Copy link
Member

seanpoulter commented Dec 5, 2017

I'm a bit confused by the code coverage highlighting. Is it working as expected? /cc @bookman25?

Here's the HTML reporter compared to our overlay:
2017-12-05 -- vscode-jest - code coverage annotations

Steps to Reproduce

Here's the repo for the example:

git clone https://github.com/seanpoulter/issue--jest-community--vscode-jest--181
@smith
Copy link

smith commented Dec 5, 2017

What does it look like when you put mapCoverage: true in your jest config?

@bookman25
Copy link
Collaborator

I think it's an issue with the coverage reporters you have set. I'm just looking at another project I have and we removed the coverageReporters config option specifically for the plugin. I can't remember the exact reason why, but it might be that jest doesn't generate the output for coverage in a way the plugin needs to consume it.

@seanpoulter
Copy link
Member Author

@smith, mapCoverage doesn't change anything.

@bookman25, does your other project look right? Any chance it's on GitHub to try the config? I've been tinkering with statement and function coverage to reproduce the HTML report. Had you tried those in the past?

@bookman25
Copy link
Collaborator

The repo is for work and private :( Not sure if it looks right, we turned the plugin off on it months ago because the project has too many tests :) and was taking too long to give feedback. At one point it was working correctly, but that was months ago and on jest 20 as well.

@seanpoulter
Copy link
Member Author

Gotcha. I've been doing some housekeeping for #177 and adding tests for the coverage overlay. The branch coverage is up next.

As for your slow project, have you tried the setting to start Jest in watch mode without running all the tests first? Using "jest.runAllTestsFirst" : false may help. The extension has a performance bottleneck waiting for the results of the rull run from the JSON reporter. We might improve performance if we streamed results after each test.

@bookman25
Copy link
Collaborator

Nice, didn't realize that was an option 👍 Coverage appears to be working as expected in my project. We're only on jest@20 so not sure if jest@21 performs differently. I think the relevant config settings we have are roughly the following. We explicitly removed collectCoverageFrom, coverageReporters and coverageThresholds for the plugin.

"jest.pathToConfig": "./jest.plugin.js",
jest.config: mapCoverage:true

const config = require('./jest.config');

delete config.collectCoverageFrom;
delete config.coverageReporters;
delete config.coverageThreshold;
config.collectCoverage = true;

module.exports = config;

@seanpoulter
Copy link
Member Author

Thanks. Now you've got me curious ...

@bookman25
Copy link
Collaborator

image
image

@bookman25
Copy link
Collaborator

I think I might be able to guess what's happening. Since it's not taking the else, it might be that it's highlighting the implicit uncovered else in your example. Also the plugin doesn't currently highlight functions either. It only does lines/branches for highlighting at present.

@seanpoulter
Copy link
Member Author

@bookman25, are you OK if I refactor overlay.ts as a class so it's easier to test? I'm just getting familiar with Jest's mocking, but it seems let testing a lot easier to manage mocks when you're checking behavior (e.g.: callFoo() calls foo().

// module.test.ts
import m from './module'

it('calls foo', () => {
  (m.foo as any) = jest.fn()
  m.callFoo()
  expect(m.foo).toBeCalled()      // Nope: Expected mock function to have been called
})

it('is not a mock', () => {
  expect(m.foo.mock).not.toBeDefined()   // Nope: Expected value not to be ... a mock!
})


// class.ts: OK
import { C } from './class'

it('calls foo', () => {
  const c = new C()
  c.foo = jest.fn()
  c.callFoo(args)
  expect(c.foo).toBeCalledWith(args)
})

it('is not a mock', () => {
  const c = new C()
  expect(c.foo.mock).not.toBeDefined()
})

Full disclosure: That's how I've been adding tests but I wanted to check in before I bother with a PR.

@bookman25
Copy link
Collaborator

I'm all in favor of adding more tests. I'm not attached to any of the code. If you find a way to improve the code so it makes more sense and is easier to maintain, then by all means feel free.

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

No branches or pull requests

3 participants