thlorenz / proxyquire

🔮 Proxies nodejs require in order to allow overriding dependencies during testing.
MIT License
2.75k stars 100 forks source link

proxyquire not stubbing module function #236

Closed samuela closed 5 years ago

samuela commented 5 years ago

Check out this PR (https://github.com/oclif/config/pull/70) at this commit (https://github.com/oclif/config/pull/70/commits/eb4b9156e5a5e887e364342c23596deff3f27790). The ts-node.ts module looks something like:

function registerTSNode(root: string) {
  const tsconfig = loadTSConfig(root)
}

function loadTSConfig(root: string): TSConfig | undefined {
  console.log('loadTSConfig unstubbed')
  // function to be stubbed...
}

export function tsPath(root: string, orig: string | undefined): string | undefined {
  registerTSNode(root)
}

and I'm requiring it with proxyquire as follows:

const stubLoader = (_: any) => {
  console.log('loadTSConfig proxyquire')
  return config
}
const tsNodePlugin = proxyquire('../src/ts-node', {'loadTSConfig': stubLoader})

// This prints "loadTSConfig unstubbed" not "loadTSConfig proxyquire"!
tsNodePlugin.tsPath('poop', 'asdf')

But unfortunately proxyquire is not actually injecting stubLoader into the module as expected.

I've tried:

bendrucker commented 5 years ago

Hi, proxyquire operates on modules, and not by munging your local scope within a module. You can't stub something in the local scope of the module under test. You can stub something that your module under test requires. We believe this is important to maintainable tests.

If your tests can manipulate the local variables of modules under test, there is no isolation between your tests and your source code. Changing an internal function name within your source code should not break tests.

If you'd like to use proxyquire, I'd recommend pulling that function out to its own module. You'll then stub it by its path relative to the module under test, and not its identifier (which is private).

proxyquire('../src/ts-node', {'./load-ts-config': stubLoader})
samuela commented 5 years ago

Understood, thank you for the quick response.