Injecting code/comments after transformation - c8 coverage fails

[markwylde]

Thanks for all your work on sucrase. It's an amazing project.

I'm a big fan of c8 for code coverage checking, and it works almost perfectly with sucrase.

Unfortunately, when you are importing other modules/files, sucrase introduces some branching logic, when it inserts this line:

"use strict";const _jsxFileName = "src/index.tsx"; function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }var _react = require('react'); var _react2 = _interopRequireDefault(_react);

Note the obj.__esModule ? obj : { default: obj } is the specific part that fails c8

This results in my code coverage claiming line 1 has untested branching logic.

If I put the following hack in, it works perfectly:

This is no longer working, see this comment below

diff --git a/src/index.ts b/src/index.ts
index feb4362..1e2329b 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -54,6 +54,9 @@ export function transform(code: string, options: Options): TransformResult {
         sourceMap: computeSourceMap(result.code, options.filePath, options.sourceMapOptions),
       };
     }
+
+    result.code = `/* c8 ignore next */\n${result.code}`;
+
     return result;
   } catch (e) {
     if (options.filePath) {

But obviously this isn't a great solution. Do you have any recommendations on how we could:

1. Remove that branch logic or the _interopRequireDefault entirely
2. Or, allowing adding custom prepends to transformation results. e.g.:

require("sucrase/dist/register").registerAll({
  prependCode: `/* c8 ignore next */`
})

Example

My ReactFast project shows the problem if you run npm run test. You'll see a coverage report claiming line 1 is uncovered.

[alangpierce]

Hey @markwylde , thanks for the report. I think ideally Sucrase would integrate well with other tools by default, so I'd certainly be open to including /* c8 ignore next */ comments by default (except with production: true) if they make c8 coverage more accurate.

I tried playing around with it myself (thanks for sharing an example repo!) and had some trouble narrowing it down. I tried changing the output code like you did, and I also tried changes to https://github.com/alangpierce/sucrase/blob/main/src/HelperManager.ts where the helpers are defined. A few observations:

• I couldn't figure out a place to put /* c8 ignore next */ or /* c8 ignore start */ in the interopRequireDefault helper code to avoid the uncovered line, but maybe there's a way to make it work.
• Adding just a newline (no /* c8 ignore next */) to the start of the file seemed to make the problem go away.
• Removing the snippet .replace(/\s+/g, " ") (which removes newlines and spaces for helpers, making them one long line) seemed to make the problem go away.
• Editing App.tsx to have a blank line at the top seemed to make the problem go away.

I'm a bit confused by those last three behaviors since it looks like AST-equivalent files are giving different results. I'm now wondering if this is a c8 bug or a situation where c8 decides to give up on lines that are too long, or something like that.

In any case, one concern with adding a newline at the start of the code is that Sucrase guarantees that line numbers are the same before and after transpile (e.g. so stack traces are accurate even without source maps). Adding extra newlines to the top of the file would break that guarantee.

If you wanted to track down the problem a bit more and figure out a concrete code change, I'd probably be happy to accept a PR that makes Sucrase more c8-friendly. I probably won't have time to dig much into this issue much myself, though.

As a separate note, if you're trying to work around this in a more immediate timeframe, I think your best option is to write your own version of the addHook function from https://github.com/alangpierce/sucrase/blob/main/src/register.ts and then include that file with -r. (Your version will probably need to be written in plain JS.) It's just ~15 lines, and you can modify transformedCode there if you want without needing any changes in Sucrase.

[fdc-viktor-luft]

I ran into exactly the same issue with

transform: { "\\.tsx?$": "@sucrase/jest-plugin" },

in my Jest config and the default coverage provider being "babel".

But I cannot tell why it works in one project configured the same way and fails in the other. Both of these projects have 100% coverage threshold for everything. Both projects use

"@sucrase/jest-plugin": "^2.2.0",

[markwylde]

Thanks @alangpierce for your detailed response. I really appreciate it and sorry I haven't responded.

I keep coming back to this, hacking away at different solutions, but can't really come up with something that feels right.

For some reason my previous solution, above, doesn't work anymore. Might be a bug with c8 not handling ignores.

Instead, I'm now doing:

diff --git a/src/HelperManager.ts b/src/HelperManager.ts
index a4879c9..7e24e18 100644
--- a/src/HelperManager.ts
+++ b/src/HelperManager.ts
@@ -23,6 +23,8 @@ const HELPERS: {[name: string]: string} = {
     function interopRequireDefault(obj) {
       return obj && obj.__esModule ? obj : { default: obj };
     }
+    _interopRequireDefault({});
+    _interopRequireDefault({ __esModule: {} });
   `,
   createNamedExportFrom: `
     function createNamedExportFrom(obj, localName, importedName) {

This fixes the issue, and confirms my initial thought that c8 is reporting correctly, the _interopRequireDefault function is basically not getting covered.

Again, it's not a pretty solution, and I'm still struggling to come up with better ways.

@alangpierce have you had any ideas/thoughts about the "HelperManager" and whether eventually those could be deprecated and instead actually transform the code to return a predictable object? It sounds like an incredibly hard thing to do, and at this stage I'm not even sure if possible.

[eventualbuddha]

I'm running into this too with the default coverage tooling for Jest (istanbul). My workaround so far is to ignore anything that looks like a helper in the first line of the transpiled source:

/**
 * Inserts comments to ignore inserted helper functions.
 */
function ignoringHelperFunctions(code: string): string {
  const firstLineEnding = code.indexOf('\n');
  const firstLine = code.slice(0, firstLineEnding);
  const rest = code.slice(firstLineEnding);

  return (
    firstLine.replace(/\bfunction _/g, '/* istanbul ignore next */function _') +
    rest
  );
}

function process(src: string, filename: string) {
    …
    const codeIgnoringHelperFunctions = ignoringHelperFunctions(code);
    return {
      code: `${codeIgnoringHelperFunctions}\n${suffix}`,
      map: sourceMap,
    };
}

It seems like we could cover (no pun intended) most of the coverage issues with this plugin just by doing something like the above. Could even throw in the c8 comment for good measure.

[fdc-viktor-luft]

I like the idea of just adding those comments here, but doesn't this require to update the source maps, too? 🤔 I guess, it requires some investigation 😅

[eventualbuddha]

This is how babel-jest handles it: https://github.com/facebook/jest/blob/3a8696bcf0e02a5f59080123d1a20e8b6d66d8df/packages/babel-jest/src/index.ts#L52

[fdc-viktor-luft]

I tested it again with

• "@sucrase/jest-plugin": "^3.0.0"
• "sucrase": "^3.28.0"

The issue doesn't appear for me anymore 🙃