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.

Sign up for GitHub

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

Jump to bottom

fix for decorator coverage #488

Merged
merged 8 commits into from
Apr 18, 2018
Merged

fix for decorator coverage #488

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e4fd8d1
add /* istanbul ignore next */ to decorators when /* istanbul ignore …
gstamac Apr 12, 2018
dcd3432
added support for ignoreCoverageForAllDecorators setting
gstamac Apr 12, 2018
f92a728
added ignoreCoverageForDecorators config parameter
gstamac Apr 16, 2018
60dac3c
Version bump
gstamac Apr 16, 2018
d8d096f
Merge branch 'master' into master
gstamac Apr 17, 2018
0253ac2
Add disclaimer for ignoring decorators
kulshekhar Apr 18, 2018
522385a
Bump version to 22.4.4
kulshekhar Apr 18, 2018
039cb69
Add @gstamac to the author list
kulshekhar Apr 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,22 @@ If you want to enable Syntactic & Semantic TypeScript error reporting you can en
}
```

### Ignore coverage on decorators
If you want to ignore coverage on decorators you can enable this through `ignoreCoverageForDecorators` and `ignoreCoverageForAllDecorators` flags. If you enable the first option you have to add the `/* istanbul ignore decorator */` comment after the decorator. If you choose the second option all decorators will be ignored.

```json
{
"jest": {
"globals": {
"ts-jest": {
"ignoreCoverageForDecorators": true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name for this configuration option is a bad choice. I'd recommend something such as enableDecoratorIgnoreComment. Maybe this should simply be true always and not configurable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is basically a fix for using typescript with istanbul I think both should default to true.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not set to true by default because ts-jest is not the right place for this. It is only being put in here because it seems like the fastest option to help address this issue and this shouldn't affect users who don't care about this part

"ignoreCoverageForAllDecorators": true
}
}
}
}
```

## Use cases

### React Native
Expand Down
2 changes: 2 additions & 0 deletions src/jest-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,6 @@ export interface TsJestConfig {
enableInternalCache?: boolean;
enableTsDiagnostics?: boolean;
disableSourceMapSupport?: boolean;
ignoreCoverageForDecorators?: boolean;
ignoreCoverageForAllDecorators?: boolean;
}
18 changes: 16 additions & 2 deletions src/preprocessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,28 @@ export function process(
fileName: filePath,
});

let tsTranspiledText = tsTranspiled.outputText;
if (tsJestConfig.ignoreCoverageForAllDecorators === true) {
tsTranspiledText = tsTranspiledText.replace(
/__decorate/g,
'/* istanbul ignore next */__decorate',
);
}
if (tsJestConfig.ignoreCoverageForDecorators === true) {
tsTranspiledText = tsTranspiledText.replace(
/(__decorate\(\[\r?\n[^\n\r]*)\/\*\s*istanbul\s*ignore\s*decorator(.*)\*\//g,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's ts-jest which is parsing the /* istanbul ignore decorator */ comments, maybe it should be named /* ts-jest ignore decorator coverage */.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment is actually processed by istanbul and not by ts-jest so the current form is correct

'/* istanbul ignore next$2*/$1',
);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you tell me what this else block is doing?

Ideally, this shouldn't change any behavior for folks who haven't set this flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I made two options to ignore decorators. One is with setting the config param ignoreCoverageForAllDecorators. Thi swill ignore all decorators and it's useful for Angular where you don't have control over the code. This is described in #454. But the other option is to add the /* istanbul ignore decorator */ and it will ignore only this decorator. I prefer the second option because I like to have control over which part of code I want to ignore in coverage. Both are optional and if you don't use them it will not modify anything.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was that if tsJestConfig.ignoreCoverageForAllDecorators is not set, the source code should not be modified for decorator coverage at all. In this case, it looks like it is being modified which will affect even users who haven't set this flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add another config for that. There are basically 3 scenarios. First is for users who don't want to change anything, second is for someone like me who want's to control which decorators are ignored and the third is for users who want to ignore all decorators. The last one is enabled by ignoreCoverageForAllDecorators config param. The first and second I didn't make any config setting but you control it by providing (or not) the /* istanbul ignore decorator */ comment. But I can add another config param for that if you believe it is necessary.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be the way to go to avoid any side effects for users who aren't setting the flag at all.

I'll take a look at this again after work and merge it if it's ready (flags, docs and updated version patch). Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Updated the readme too.

const postHook = getPostProcessHook(
compilerOptions,
jestConfig,
tsJestConfig,
);

const outputText = postHook(
tsTranspiled.outputText,
tsTranspiledText,
filePath,
jestConfig,
transformOptions,
Expand All @@ -71,7 +85,7 @@ export function process(
const modified =
tsJestConfig.disableSourceMapSupport === true
? outputText
: injectSourcemapHook(filePath, tsTranspiled.outputText, outputText);
: injectSourcemapHook(filePath, tsTranspiledText, outputText);

flushLogs();

Expand Down