could we make --all ignore comments

[bcoe]

Because our --all functionality does not take into account multi-line comments, we end up with a fairly large discrepancy between nyc and c8's --all, see:

googleapis/nodejs-asset#236

I wonder if we could be clever and detect multiline comments, and drop these from our estimation of --all.

[bcoe]

CC: @j03m.

[bcoe]

I did a bit of digging into this last night ... I wonder if it's worth the trouble, we end up going down the path of building a parser, and need to move around quite a bit of code in v8-to-istanbul. Maybe just treating each line as missing is correct, in the case of not exercising code.

[j03m]

Yea, this isn't great. Looking at this, my current understanding is that we wouldn't have this issue for a file that is loaded, because in that case we're not manufacturing the coverage data? Is that correct?

If that understanding is correct I think we could be more sophisticated. We could in theory use a parser like babylon or recast and parse the file into an AST and then walk the AST to generate the uncovered sections. This would be more expensive then the line stat we do today, but it would be much more accurate.

What do you think?

[bcoe]

My current thinking, argue against it 😆, is that if you haven't loaded the file you haven't exercised the lines comprising comments ... so perhaps we're fine as is.

[schmidt-sebastian]

Note that we had to manually exclude .d.ts files from the coverage runner, as they are only used during the transpile stage and never loaded at runtime. We use them to declare external modules and our own types.

[j03m]

Will take a look at this.

[gboer]

Just adding my 2 cents here, this also relates to files that only have Typescript interfaces. When executing the code with ts-node, the interfaces disappear, since they are not native to Javascript and then end up being uncovered in the final report (with --all enabled). However, it is also impossible ignore them using c8-ignore statements, since (I assume), are not read at all. So the only way I can exclude them is by disabling --all.

[kf6kjg]

I'd love to have c8 be able to detect the usage of @ts-expect-error/@ts-expect-no-error and how they result in checking the various branches of a type. However c8 is fundamentally about JavaScript/ECMAScript, not Typescript. Thus the second best option would be to do as requested here: just ignore any lines that don't contain actual runtime JS when the --all flag is set.

My barely-thought-out suggestion: only count actual runtime JS code in the covered or uncovered percentages. Other lines are treated as if they don't exist. That way large amounts of TS type definitions don't artificially pad or depress the numbers.

[darcyrush]

this also relates to files that only have Typescript interfaces.

Note that we had to manually exclude .d.ts files from the coverage runner

So I had the exact same issue of --all showing a .ts file which only contained TS types as uncovered, and after reading both comments I realized I could just rename the file extension to .d.ts which resolved my coverage issue and made the file more descriptive.