Using `import:` to mock globals messes up coverage (with c8)

[bosschaert]

If I use import: in esmock to mock a global it messes up the coverage report. At least it does when run with c8 mocha. I constructed a minimal test scenario for it below:

Here's an example source file src/myfile.js:

function someFun() {
  console.log('something');
}

export async function testFun() {
  console.log('test');

  return 418;
}

And here's an example test for it. It only tests the testFun method, but it has a mock defined for the global fetch (which isn't actually used):

import assert from 'assert';
import esmock from 'esmock';

describe('ESMock test case', () => {
  it('Simple test', async () => {

    const { testFun } = await esmock(
      '../src/myfile.js', {
        import: { fetch: async (url) => {} }
    });

    assert.strictEqual(418, await testFun());
  })
});

When I run this and look at the coverage report it looks like this:

The above coverage report is clearly wrong.

If I comment out the line with the import, e.g.:

    const { testFun } = await esmock(
      '../src/myfile.js', {
        // import: { fetch: async (url) => {} }
    });

the coverage report is correct again:

Note that using esmock with things other than globals, doesn't have this issue, e.g. the following doesn't cause this issue:

    const { postSource } = await esmock(
      '../../src/source.js', {
        '../../src/included.js': {
          default: mockResp
      }
    });

It would be great if I can use the globals mocking without messing up the coverage report 😄

[iambumblehead]

if there's no objection, I'd like to close this issue to merge with an earlier similar issue #271 (will wait for reply)

There is a long-outstanding issue at the c8 repo for this bcoe/c8#325

The maintainer requested a reproduction and I submitted one, but there was no follow-up bcoe/c8#501 possibly the solution would need to come in multiple separated PRs

[iambumblehead]

I'm surprised the coverage report is correct when modules and not globals are mocked

[bosschaert]

I have no problem with merging this with #271 but it would be nice to describe this import: related problem there then too. I did read that issue before reporting this one and thought it was about a different problem.

To me it sounds like that these are 2 different problems that might have the same underlying cause?

[iambumblehead]

If you recommend any changes the title or anything else around that ticket they can be made.

To me it sounds like that these are 2 different problems that might have the same underlying cause?

Okay, keep this issue open

For mocking global values, esmock returns a modified version of source file(s) to node's loader; where the global value being mocked is defined at the top of the file. Maybe that change would need to be coordinated with the coverage tool in some way, but to be honest I don't know how to solve the issue.

const {fetch} = global.esmockCacheGet("treeid5"); // <- esmock adds this
// rest of file

Any ideas for a solution?

[bosschaert]

Any ideas for a solution?

One thing I've been doing as an alternative way to mock these globals is to use globalThis.

For example something like this if I want to replace the global fetch:

    const mockFetch = async (url) => { ... }; // My mock fetch function

    const savedFetch = globalThis.fetch; // Save the original global fetch
    try {
      globalThis.fetch = mockFetch; // replace the global fetch with my mockFetch

      // Now call my function that calls fetch under the covers
      // it will use the mockFetch
      assert.strictEqual(418, await testFun());
    } finally {
      globalThis.fetch = savedFetch; // put the original global fetch back in the finally block
    }

You can make these changes to globalThis after the source file being tested is loaded, so you could put something like this in the logic of await esmock(...) call without the need to alter the original source file.
The only thing that needs to be done extra is to reset the original values, which I do in a finally block. Maybe an extra call something like esmock.resetGlobals() could do this, but there might be a smarter way to do that somehow.

[iambumblehead]

So it seems supporting globalThis and esmock.resetGlobals() would require esmock to lose its ability to be used concurrently OR esmock would need to provide an extra interface and logic that conditionally use globalThis and implicitly requires sequential usage from the user.

Maybe support for a second "import"-like namespace, such as "globalThis", could be added in order to use globalThis for mocking.

One of the main benefits of esmock imo is that it can be used with concurrent tests and so using globalThis is a downgrade in that regard.

If you are around feel free to chime in @koshic

Also "setting" and "clearing" properties of globalThis could be handled with a seperate file or package and does not actually need to be handled by esmock. However, for users who would want to use globalThis, it would be convenient if esmock could help with that.

[iambumblehead]

I wish there were a way to avoid using globalThis and to get correct results from c8 here but don't know if, or how, it could be achieved.

[iambumblehead]

node.js' native coverage tooling now allows one to specify files for coverage analysis nodejs/node#53553

Have not tried it myself yet, but perhaps native coverage results do not exhibit this same issue

[iambumblehead]

another we could try doing is; putting the lines added by esmock onto the existing first line of the file with no additional newlines. esmock is already doing this

[iambumblehead]

this issue also happens using node's native test coverage tools.

another idea is to add 'global' definitions to the bottom of the file using "var" so to make the definitions available at the top of the file

[koshic]

@iambumblehead can /* c8 ignore start */ / /* c8 ignore end */ around injected code help us for c8 use case? It's a question (may be a bit stupid...), I didn't try it yet )

[iambumblehead]

I experimented with adding using var but the definitions are not "hoisted" the way that I expected.

This results in a runtime error where dateWrapper becomes undefined

const dateWrapper = Date

export {
  dateWrapper
}

var Date = { now: () => 'now' }

changing 'var' for const or let results in ReferenceError [Error]: Cannot access 'Date' before initialization

[iambumblehead]

@koshic thanks for the suggestion that might be something to try. wondering if there is something similar for native node test coverage tools.

[iambumblehead]

ime usage of c8 ignore removes sections of code from being included in the report, but it does not elide those lines from the analysis in a way that would shift the lines being reported

[iambumblehead]

I also tried replacing "const" with " var" to try and get the hoisting behaviour... but var-defined dateWrapper is still 'undefined'

sourcesafe.replace(/const/g, '  var')

I'm all out of ideas