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: exclude generated code from code coverage instrumentation #8226

Conversation

AriPerkkio
Copy link

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

Fixes #7824.

For more context I recommend to read the issue description of #7824 and test out the reproduction setup from https://github.com/AriPerkkio/svelte-istanbul-reproduction/blob/main/debug-svelte-sourcemaps.mjs.

There hasn't been much action on the issue itself so I'm not sure whether this approach is accepted. Though I think this is the only possible approach as the lines cannot be excluded from source maps either - described in the linked issue in more details. Feel free to reject and suggest alternative solution. I have had no experience with Svelte compiler before.

Before:
Screenshot 2023-01-25 at 14 27 54

After:
Screenshot 2023-01-25 at 14 27 08

@benmccann benmccann changed the title [fix] Exclude generated code from code coverage instrumentation fix: exclude generated code from code coverage instrumentation Jan 28, 2023
@baseballyama
Copy link
Member

Thank you for your PR!
IMO I don't think this PR is the right fix for the following reasons:

  • I think this approach will eventually fill the compiled code with istanbul ignore next. But Svelte should not be dependent on any particular coverage tool.
  • As a testing strategy, instead of checking the coverage of the Svelte template section, IMO it's better to check the DOM structure or do VRT (Visual Regression Testing).

@AriPerkkio
Copy link
Author

  • As a testing strategy, instead of checking the coverage of the Svelte template section, IMO it's better to check the DOM structure or do VRT (Visual Regression Testing).

I think coverage is not really a testing strategy, rather a metric describing your tests. It does not matter if you are checking structure of DOM or visual regression - it just describes how many branches and statements your tests triggered. It could be useful to have coverage enabled when doing visual regression tests just to see how much is covered by that kind of testing.

Some companies require tests to reach certain threshold in code coverage. The requirement may even come from a higher level specification/standard etc. With Svelte codebases it's difficult to reach full coverage even if all source code is executed.

I think this approach will eventually fill the compiled code with istanbul ignore next

I completely agree this approach isn't ideal. It would be great if those generated parts could be just excluded from source maps so that these coverage ignore hints were not needed. I've seen other code transforming tools using these ignore hints before. I'm quite sure some babel plugin used to add these but cannot find it anymore. Here is one example how ts-jest used to add exclusions for Typescript's decorators: kulshekhar/ts-jest#488.

@AriPerkkio
Copy link
Author

It's actually possible to achieve this without using ignore hints at all. The generated code just has to be restructured a bit. After doing some changes in src/compiler/compile/render_dom/wrappers/shared/Tag.ts I achieved following output, and now the ctx[1] + "" part that is included in source maps is outside of the if-block. I'll set separate PR with these changes once implementation is refactored.

p(ctx, dirty) {
+   let t_value_updated = /*user*/ ctx[1] + "";
+   if (dirty & /*users*/ 1 && t_value !== (t_value = t_value_updated)) set_data(t, t_value);
-   if (dirty & /*users*/ 1 && t_value !== (t_value = /*user*/ ctx[1] + "")) set_data(t, t_value);
  },

Debugging should still work:
image

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.

Help coverage instrumenters exclude compiler's helper functions
2 participants