Istanbul coverage interference

[cameron-martin]

This is the same issue as in upstream: eddyerburgh#37. It looks like your fork has the same problem too.

My suggestion would be to serialise functions as some placeholder, possibly including the function's name if it is available. For example [Function foo].

[TheJaredWilcurt]

@cameron-martin okay so I made a branch for this:

• Istanbul #72

And everything is done and ready to be merged, but for some reason the indentation is changing when ran on the CI. I tested it on Windows 10, Windows 7, and Ubuntu 17. And they all work fine locally (git clone, git checkout instanbul, npm install, npm t). But for some reason the inline functions are indented differently when ran on the GitHub CI (Ubuntu 20). I could use some help on this. I don't want to release something that breaks snapshots depending on the OS they were created.

[cameron-martin]

Since the CI on that PR now passes, does this mean the issue is fixed?

I checked out the failing commit, 7a05e64, and that passed for me on my local machine which is also ubuntu 20.04.

[TheJaredWilcurt]

Yeah, I think I got it to work. The issue was that the tests can only pass when you run jest with coverage (istanbul) on. Otherwise the Istanbul bug isn't present and you so get a different snapshot. This will still happen, but it already did anyways. I experimented with some ideas but couldn't find a way to fix it on my end.

If other people have this issue you they could try to skip tests that they know will fail with coverage disabled by doing this:

function coverageIsActive () {
  let coverage = false;
  process.argv.forEach(function (arg) {
    if (arg === '--coverage=false') {
      coverage = false;
    } else if (arg.startsWith('--coverage') {
      coverage = true;
    }
  });
  return coverage;
}

if (coverageIsActive()) {
  test('My test that fails when coverage is false', () => {
    let wrapper = mount(MyComponent);

    expect(wrapper)
      .toMatchSnapshot();
  });
}

Since this has to be done in your code base on your test, I couldn't figure out a way to page it into the Istanbul fix feature in this library.

But at least this will fix the fact that it breaks even with coverage enabled.

[cameron-martin]

Does this mean you still get different snapshots when coverage is enabled vs disabled when this setting is enabled?

[TheJaredWilcurt]

Yes, and I do not know of any way to fix that from a library perspective, other than maybe checking if --coverage=false and then adding 8 spaces of indentation manually, but all of this string manipulation of the snapshot comes before the beautify settings are applied which adjust indentation type (2 space, 4 space, tab, etc). Open to PR's if someone can figure out a clean way to resolve this, but we may be better off just telling Istanbul to cut it out, instead of trying to bandaid over what they're doing.

[TheJaredWilcurt]

The removeIstanbulComments flag has been added to the API and is enabled by default in v3.16.0. Closing this issue.

[cameron-martin]

That's unfortunate since the issue here, at least for me, is not the additional noise in snapshots caused by istanbul, but that you can't have tests pass both with coverage enabled and disabled.

My original suggestion was to not include the source code of functions at all in the snapshots, instead replacing them with a placeholder. This would solve my issue. Is this something you would consider?

[TheJaredWilcurt]

@cameron-martin You can use the attributesToClear: ['title', 'alt'] feature if the attributes are known and specific. Otherwise we'd need to change the functionality of removeIstanbulComments to allow passing in a specific string to denote replacing the value of the attribute entirely. This would require some regex magic to detect if the attribute value matches the pattern of a function before replacing it with a placeholder. Doable though.

It may make more sense to have this be it's own boolean in the API, like clearInlineFunctions and default it to false.