[v4.0] coverage map #588
[v4.0] coverage map #588
Conversation
Pull Request Test Coverage Report for Build 783
💛 - Coveralls |
|
ping @seanpoulter |
There was a problem hiding this comment.
Wow, sorry for the slow review. I still haven't taken time to run it :/.
It looks good. The optimization when the visibility changes looks a bit backwards to me, but otherwise another job well done. 👏 Ship it!
Should we notify the folks on the issues to beta test it for you?
src/Coverage/CoverageMapProvider.ts
Outdated
| if (!visible) { | ||
| this.init(); | ||
| } |
There was a problem hiding this comment.
I'm probably overlooking something, but it seems funny to reset the coverage and source map when the document is no longer visible. I would have expected that we remove the entry from the map.
There was a problem hiding this comment.
the visibility here means the coverage, it is not visible when people toggle the coverage off. And when that happens we remove all maps in the init method by reinitializing the map. Maybe the name is confusing? it might be more accurate to call it reset.
| }); | ||
| }); | ||
|
|
||
| describe('getFileCoverage()', () => { | ||
| it('should return the file coverage if found', () => { | ||
| it('should return the file coverage if found', async () => { | ||
| expect.hasAssertions(); |
| { pattern: '**/*.{ts,tsx,js,jsx}' }, | ||
| new CoverageCodeLensProvider((uri) => extensionManager.getByDocUri(uri)) | ||
| ), | ||
| vscode.languages.registerCodeLensProvider(languages, extensionManager.coverageCodeLensProvider), |
There was a problem hiding this comment.
Nice. I appreciate that you've cleaned this up.
| }); | ||
| it('visibility = true => no-op', () => { | ||
| const sut = new CoverageMapProvider(); | ||
| jest.clearAllMocks(); |
There was a problem hiding this comment.
Can we pull these two clearAllMocks() out into one beforeEach?
There was a problem hiding this comment.
the following 2 methods are called in both the constructor and the onVisiblityChange, thus reset the mock after constructor, but there is another way to clear the confusion, I will just call the expect in both places...
| expect(coverageCodeLensProvider.coverageChanged).toBeCalled(); | ||
| expect(sut.coverageOverlay.updateVisibleEditors).toBeCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Hah, that's a lot of nulls changing. At some point (not in this PR) we should consider wrapping the constructor and using default parameters:
const extension = ({
context = null,
coverageCodeLensProvider = null,
debugCodeLensProvider = null,
debugConfigurationProvider = null,
failDiagnostics = null,
instanceSettings = null,
jestWorkspace = null,
outputChannel = null,
pluginSettings = null,
workspaceFolder = null,
}) => new JestExt(
context
workspaceFolder,
jestWorkspace,
outputChannel,
pluginSettings,
debugCodeLensProvider,
debugConfigurationProvider,
failDiagnostics,
instanceSettings,
coverageCodeLensProvider,
);so we end up with an idea what's null or renamed:
const sut = extension({
coverageCodeLensProvider
debugCodeLensProvider,
debugConfigurationProvider,
jestWorkspace: projectWorkspace,
outputChannel: channelStub,
pluginSettings: extensionSettings,
workspaceFolder,
});
|
@seanpoulter nice to see you reviewing this, I will take a look tomorrow, thanks. |
Co-authored-by: Sean Poulter <sean.poulter+gh@gmail.com>
Co-authored-by: Sean Poulter <sean.poulter+gh@gmail.com>
…ode-jest into coverage-upgrade
yeah, we should build a beta soon so people can test it, but I want to include the matching logic as that is a much bigger change, don't want people to test it too many times... |
reference v4 #576 coverage map issue
change summary
mapStore.transformCoveragehas become async operation, therefore we need a way to refresh the coverage codeLens and overlay asynchronously (seeJestExt. _updateCoverageMap)fix #559
fix #551
fix #524