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

Question / Bug - Unable to mock commonjs dependencies #312

Open
Filipoliko opened this issue Oct 2, 2024 · 10 comments
Open

Question / Bug - Unable to mock commonjs dependencies #312

Filipoliko opened this issue Oct 2, 2024 · 10 comments

Comments

@Filipoliko
Copy link

Hi,

thank you for this great library, thanks to this, we were able to actually use native node test runner in some of our projects!

I ran into an issue, but maybe it is expected behaviour. When I am trying to mock a dependency of a dependency, it does not really work. Maybe the issue is that both dependencies are commonjs modules?

I prepared a reproduction repository - https://github.com/Filipoliko/esmock-bug - but the use-case is quite straightforward. Ultimately, I am trying to mock fs module globally (even within dependencies like changelog-parser), but I am failing to do so. Being desperate, I tried to mock line-reader module, which is the one using the fs module in my case, but this failed also. This is a simplified example, where I was unable to mock the 3rd party dependency of a dependency.

Code

import { test, mock } from 'node:test';
import assert from 'node:assert';

import esmock from 'esmock';

test('parseChangelog', async () => {
    const content = 'content';
    const parseChangelog = await esmock('changelog-parser', {}, {
        fs: { // This does not get mocked in `line-reader` module
            open: mock.fn(),
            close: mock.fn(),
            read: mock.fn(() => content),
        },
        'line-reader': { // To my surprise, even this does not get mocked in `changelog-parser` module
            readLine: mock.fn(() => content),
        }
    });

    // This leads to a real `line-reader`.readLine call, which leads to `fs.open`
    const changelog = await parseChangelog({
        filePath: 'fake',
    });

    assert.equal(changelog, content);
});

Output (Node 22.9.0)

$ node --test

✖ parseChangelog (32.4075ms)
  [Error: ENOENT: no such file or directory, open 'fake'] { errno: -2, code: 'ENOENT', syscall: 'open', path: 'fake' }

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 358.095541

✖ failing tests:

test at repro.test.mjs:6:1
✖ parseChangelog (32.4075ms)
  [Error: ENOENT: no such file or directory, open 'fake'] { errno: -2, code: 'ENOENT', syscall: 'open', path: 'fake' }

The result is, that there is no mocked fs, nor line-reader and the test really tries to open fake file, which fails.

I am trying to use memfs as a replacement for native node fs, since we are trying to create some sort of an integration test.

@iambumblehead
Copy link
Owner

Adding console.log({ parentURL }) to esmockLoader's resolve function normally logs every module that is loaded, however in the reproduction, the conole.log is never called again when changelog-parser is loaded.

This is un-expected because the resolve function is a hook function called directly by node.js.

I've tried various things but with no success. Tried specifying type: "module" at the reproduction package.json, tried importing changelog-parser indirectly through an additional "import-then-export-change-parser.js" file, tried removing the content of changelog-parser/index.js and replacing it with something that imports "fs" and calls fs.read, tried updating the test to only mock node:fs... no luck.

The full tests from a fresh cloned esmock are passing here, so the module loader hooks are working normally for many tests there.

Not sure what the solution is but I will keep thinking about it.

Thanks for opening the issue and creating the reproduction sample.

@iambumblehead
Copy link
Owner

iambumblehead commented Oct 2, 2024

I added the test inside the existing esmock test files and it fails there. I removed the contents of changlog-parser's index.js to be sure that the readline package is not interfering with the terminal output in some way.

Added this inside the "load" hook function and when changelog-parser is loaded, the first the console.log appears in the process output, but the subsequent calls are not seen...

  console.log('PASS1', { url })
  console.log('PASS2', { url }) // not logged
  console.log('PASS3', { url }) // not logged

My intuition is a simplified test could probably be made to demonstrate this as a node.js bug, but I don't have bandwidth at the moment to make one. Perhaps, under certain conditions, node.js wrongly continues the stack before module-loader hooks have returned values.

If you don't mind using unstable newer releases of node, you could try this new module mocking functionality nodejs/node#52848

@Filipoliko
Copy link
Author

Thank you for your quick reply, I tried using the native mock.module, but it has some other issues for a change 😃

nodejs/node#49442 (comment)

@iambumblehead
Copy link
Owner

Thanks for the update I hope a solution will appear

@iambumblehead
Copy link
Owner

iambumblehead commented Oct 4, 2024

readline imports fs inside of an iffe and that may be what causes it to early-load fs before it is processed by the module-loader

(function () {
  var fs = require('fs')
  // ...
}());

@Filipoliko
Copy link
Author

The very same use-case seems to be happening with graceful-fs library. I also cannot mock the fs dependency inside for some reason.

import { test, mock } from 'node:test';
import assert from 'node:assert';

import esmock from 'esmock';

test('graceful-fs', async () => {
    const content = 'content';
    const fs = await esmock('graceful-fs', {}, {
        fs: {
            readFileSync: mock.fn(() => content),
        },
    });

    assert.equal(fs.readFileSync(), content);
});

Not sure if patching changelog-parser will solve the issue as this seems to be happening in more than one dependency. But thank you for trying to solve this, really appreciate it and I am amazed by your activity. The problem seems to be mostly in older libraries with little maintenance, so maybe it is time to let the old ways die and focus on the new stuff :D

@iambumblehead
Copy link
Owner

I noticed graceful-fs does not require/import readFileSync, so I tried to to test that with readFile which is required by graceful-fs, however, my attempt also failed.

Maybe I hold a misunderstanding about the way the moduleLoader works or maybe this is a bug in the nodejs runtime... the next step should be to make a smaller test isolating the inability of the 'resolve' and 'load' hooks to indertict require usages.

There are many esmock tests and it is surprising they are missing whatever is happening here.

@iambumblehead
Copy link
Owner

related nodejs/node#55307

@iambumblehead
Copy link
Owner

This single file mocks the fs module using a similar flow esmock should follow, but no success updating esmock to do this

repro.mjs
import { register } from 'node:module';
import assert from 'node:assert';
import fs from 'node:fs'

function log (...args) {
  return fs.writeSync(1, JSON.stringify(args, null, '  ').slice(2, -1))
}

async function resolve(referrer, context, next) {
  const result = await next(referrer);
  const url = new URL(result.url)

  if (result.url === 'node:fs') {
    result.url = import.meta.url + '#-#' + result.url
  }
  
  return result
}

async function load(url, context, next) {
  log('load: ' + url.slice(0, 60))

  if (url.endsWith('#-#node:fs')) {
    log('second: node:fs is imported from dummy url import.meta.url', {
      url: url.slice(0, 60)
    })

    return {
      shortCircuit: true,
      format: 'commonjs',
      responseURL: url,
      source: `
        module.exports = {
          readFileSync: () => 'test'
        }`
      }
  }
  if (url.endsWith('file.js')) {
    log('first: parent that includes node:fs is returned as cjs', {
      url: url.slice(0, 60)
    })

    return {
      shortCircuit: true,
      format: 'commonjs',
      responseURL: url,
      source: `
        const fs = require('node:fs')

        module.exports = p => fs.readFileSync(p, 'utf8')`
      }
  }

  if (url.endsWith('#-#node:fs')) {
    log('second: node:fs is imported from dummy url import.meta.url', {
      url: url.slice(0, 60)
    })

    return {
      shortCircuit: true,
      format: 'commonjs',
      responseURL: url,
      source: `
        module.exports = {
          readFileSync: () => 'test'
        }`
      }
  }

  return next(url, context);
}

register([
  `data:text/javascript,`,
  `import fs from 'node:fs';`,
  `${encodeURIComponent(log)};`,
  `export ${encodeURIComponent(resolve)};`,
  `export ${encodeURIComponent(load)}`
].join(''));

const testread = (await import('./file.js')).default

assert.strictEqual(testread('testpath'), 'test')

@iambumblehead
Copy link
Owner

iambumblehead commented Oct 8, 2024

For anyone who encounters this issue, you should be fine if your tests apply mocks to ESM targets Per the description, esmock provides: "ESM import and globals mocking for unit tests" but sample tests associated with this issue attempt to mock to import trees that are entirely commonjs.


Many subtle time-consuming adjustments are needed for esmock to possibly handle this one and there's no certainty they would be succesful.

  • It seems node's resolve hook is not always called for commonjs modules. To trigger the resolve hook for the 'graceful-fs' module, it was necessary for esmock's load hook to read that module file and to return a source string from the load hook similar to what is done in the single-file example attached to the previous comment. More testing is needed to confirm details. Simply returning nextLoad(url, context) from the load hook does not trigger the resolve hook.
  • When the builtin mock is defined at the load hook, it must define its format as "commonjs" and must use commonjs' export format. When doing this, there is cryptic runtime error message "TypeError [Error]: Cannot read properties of undefined (reading 'exports')" and ff the mock source string is modified to remove the word "exports" the error continues to occur with the same error message, so its not clear where or why this error actually originates,
    TypeError [Error]: Cannot read properties of undefined (reading 'exports')
      at require (node:internal/modules/esm/translators:248:33)
      at Object.<anonymous> (/home/bumble/soft/esdock-bug/node_modules/graceful-fs/graceful-fs.js:2:10)
  • this log was used inside the hooks as console.log is not predictable there
    const log = (...args) => (
      writeSync(1, JSON.stringify(args, null, '  ').slice(2, -1)))

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

No branches or pull requests

2 participants