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

fix for decorator coverage #488

Merged
merged 8 commits into from
Apr 18, 2018
Merged

fix for decorator coverage #488

merged 8 commits into from
Apr 18, 2018

Conversation

gstamac
Copy link
Contributor

@gstamac gstamac commented Apr 12, 2018

As discussed in #454 there is a problem with coverage on decorators when emitDecoratorMetadata is enabled. This is kind of a hack to fix this until something better is implemented in tsc/jest/istanbul. There are two options. First you can set a configuration option ignoreCoverageForAllDecorators to true and it will ignore all decorators (and possible something else). The second option is to add /* istanbul ignore decorate */ right after the decorator like this
@Decorator /* istanbul ignore decorate */

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 12, 2018

Looks good to me. Can I get you to write a test to ensure we don't have any regressions later on?

@gstamac
Copy link
Contributor Author

gstamac commented Apr 12, 2018

I was searching the code where to put the code related to coverage but couldn't find it. Could you point me to the right direction. Thanks.

@gstamac
Copy link
Contributor Author

gstamac commented Apr 12, 2018

Also I was just thinking this is a temporary solution. This should probably be solved in tsc and we could write it like this
/* istanbul ignore next */@Decorator
The problem now is that this comment gets removed by tsc even if removeComments is set to false.

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 13, 2018

Hm, maybe we don't actually have any tests for coverage - I couldn't find one at a quick glance. @kulshekhar ?

@kulshekhar
Copy link
Owner

given that coverage is not handled by ts-jest at all (jest takes care of that), I'm not sure about this

@gstamac
Copy link
Contributor Author

gstamac commented Apr 14, 2018

That's true but the comment is copied by tsc and therefore kind of part of ts-jest. The other option would be to implement it in Typescript but I wouldn't count on that.

@kulshekhar
Copy link
Owner

The problem now is that this comment gets removed by tsc even if removeComments is set to false.

Does this happen only in ts-jest or even with tsc from the command line?

@kulshekhar
Copy link
Owner

kulshekhar commented Apr 14, 2018

I just tested this with tsc and tsc allows comments through with the default settings. This means that even ts-jest will not strip these comments out.

In this case, I'm wondering whether it wouldn't be better for the actual applications to add this comment in the code themselves. On the other hand, I can see how this can be a pain - particularly for apps with a lot of decorator usage - but this could really be done with a mass search/replace so it shouldn't be a big issue

@GeeWee
Copy link
Collaborator

GeeWee commented Apr 15, 2018

I don't think the applications should add it - it'll be super-bloated. How do other applications that run through jest handle it?

@kulshekhar
Copy link
Owner

Adding this in would mean supporting it and any other edge cases this may cause. That this would be behind a flag doesn't change this.

The fact that this itself is somewhat of an edge case, related to a feature (coverage) that ts-jest has no control over makes it hard for me to justify adding this in.

@gstamac
Copy link
Contributor Author

gstamac commented Apr 15, 2018

Let's start at the beginning. @decorator gets transformed into a __decorator call by tsc. If emitDecoratorMetadata is enabled the call cannot be covered by tests that's why the coverage cannot be 100%. This is described in #454.
To ignore this coverage we would have to prepend the __decorator call with /* istanbul ignore next */ comment. Logically we would prepend the decorator with that comment (like this /* istanbul ignore next */@decorator) and tsc should output it before __decorator call. However this comment gets removed by tsc.
So there are two options as I see it. One is to change tsc to output the comment before the decorator. But I don't think this will happen soon. The other is to implement it in ts-jest which I did.
I don't see a harm in adding this since you don't have to use it. If you don't enable it by setting the config parameter or don't use the /* istanbul ignore decorator */ comment nothing changes.

'/* 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.

@kulshekhar
Copy link
Owner

I'm still not convinced that ts-jest is where this belongs.

However, as I probably won't have time to look at this again in the coming week(s), I also don't want to leave this hanging.

If you update the docs to indicate that this is experimental and could be removed later, I'll merge it in. If you also bump the version patch, I'll publish this change right after merging it.

But before doing this if you could please respond to my review question.

@gstamac
Copy link
Contributor Author

gstamac commented Apr 16, 2018

Did the version bump.

@Prior99
Copy link

Prior99 commented Apr 18, 2018

I am also struggling with this issue. Are there any plans to merge this? Or are any other workarounds known?

"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

}
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

@kulshekhar
Copy link
Owner

I've made the required changes and will take a look at this again and merge/publish it after work

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

Successfully merging this pull request may close these issues.

4 participants