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

Improve dependency mocking documentation. #2010

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 94 additions & 14 deletions docs/_howto/link-seams-commonjs.md
Original file line number Diff line number Diff line change
@@ -1,17 +1,80 @@
---
layout: page
title: How to use Link Seams with CommonJS
title: Dependency mocking with Sinon
---
This page describes how to isolate your system under test, by stubbing out dependencies.

This page describes how to isolate your system under test, by stubbing out dependencies with [link seams][seams].
While in other languages you might use [link seams][seams], [Dependency Injection(DI)][di] or
[Inversion of Control (IoC)][IoC], which are not just _patterns_ but also requires some pattern
implementation(a framework) - CommonJS(aka nodejs) module system provides a way better to replace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a framework to do DI. I do it all the time by having optional constructor parameters. If using links seams really is "better" is debatable, as it introduces a new dependency, apis, complexity and mental overhead, but at least it doesn't require you to change the SUT.

In general, you don't need any custom framework to use IoC (of which DI is one pattern). Constructor injection or service locators are simple to implement in any language. Less mystery is one of our tenants, in any case, so we wouldn't want to give people the impression you need special tools, unless you are doing "special" stuff like hooking into the module loading system (as we are doing here).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • DI. Accepting your dependencies via arguments, ie using some sort of a factory pattern(and able to produce multiple objects). That's more about runtime.
  • dependency mocking. Statically declaring your dependencies, without the ability to factor different versions. That's only about testing.

I would say - DI needs some framework, as long as you have to structure and design your code in a specific way, while dependency mocking operates on a bit different level and in a different time(tests only).

To be honest - I am quite happy that we have such ability in JS as a dependency mocking, so I don't have to implement 3 tier architecture to be able to mock fs. That makes code simpler and tests simpler.

There are more Java developers around me than JS, with all that Enterprise stuff from the fairy tales, and I like to show them how simple the code could be. (Sometime they do the same)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the opinion that dependency injection does not need a framework in JavaScript. I often use Sinon's spies, stubs and fakes in combination with pure functions. This makes my code easy enough to reason about, that I can get my colleagues to 👍 my pull requests.

any module, or package you need - _require-time_ dependency replacement.

This is the CommonJS version, so we will be using [proxyquire][proxyquire] to construct our seams.
## Why
Before we started, let's clarify why you could not use _just Sinon_ to mock.
### Source file: `file.js`
A short script, which does a quite important and a very dangerous action. Hopefully only for a `root` user.
```javascript
const fs = require('fs');
const isRoot = require('./isRoot');

const wasRoot = isRoot(); // lets just precache it! Why not?

exports.deleteEverything = function () {
if (wasRoot) {
throw new Error('only for a true root, ' + isRoot() ? 'sudoer' : 'user')
}
fs.removeAllFiles();
}
```
### Test file: `file.test.js`
Keep in mind - the test should be repeatable.
```javascript
const file = require('./file');

To better understand the example and get a good description of what seams are, we recommend that you read the [seams (all 3 web pages)][seams] excerpt from [Working Effectively with Legacy Code][legacy] before proceeding.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think removing this is improving the article. People read too little today 👴 That book is great and every seasoned dev should know about it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I shall disagree.

  • "Seams" are a quite rare thing, probably defined only at the mentioned article, and that a quite broad term.
  • while seams are "alter behaviour in your program without editing in that place", they are not about dependency mocking
  • provided examples are using or C preprocessor(not exists in JS), or Java class inheritance(might not exist in js). All examples are 100% alien to JS dev

I've read and wrote many articles related to this subject, and this one is probably most less useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the articles. To say the truth - I don't know any good one, I truly want to recommend. I could share a few ones of mine, but they are also not great, and bound to rewiremock, which is a conflict of interest of some sort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand by my recommendation of that book. Please drop this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But sooner or later it have to be replaced by a more js-related example, as long as the goal is to improve. Someone has to write it first.

The goal of the every article is to be useful for the reader, and provide some information their could use directly or indirectly, but just after reading. Unfortunately, patterns from the "3 pages" are not quite applicable to JS, or just don't exist in the same form:

  • page 2 seam - is portable to js class inheritance, almost as-is.
  • page 3 example 1(preprocessor seams) - there is no analog for preprocessor, except babel macros or require hooks, but that's not an option. The closest analog is using process.env.NODE_ENV==='test', but, again, that's not the same.
  • page 3 example 2(link seams) - seam at the linking stage is equal module dependency injection pattern.
  • page 3 example 3(object seams) - no analog, except inheritance seam from page 2, which is not the same.

So - if you have to idea how linking in C works, or what's the difference between C and JS class system(vast majority of devs) - then 0(zero) seams are applicable or understandable.

By fact there are 2 legit ways to use seams in js:

  • DI/providing callbacks into the constructor, so creating an object/calling a function, which shall be "pure", and call only provided callbacks in a response, not perform a real action. Especially not to do anything before "the call".
  • sandboxing - via class inheritance, module dependency replacement, and sinon.sandbox.
  • 😬 while devs are usually just overriding class methods or module variables "when it's too late", and that's what I am fighting with.

// you can stub deleteEverything
sinon.stub(file, 'deleteEverything');
// you can "safely" call it
file.deleteEverything();
// but how could it help?
```
In short - you can't test this file at all if you are not a root. And if you are, well, say goodbye to your file system.

Read it?
To solve tasks like this, `fs` and `./isRoot`, dependencies, not local or exported variables, should be stubbed and controlled.
Sinon could stub only something you export, not the way module is assembled.

Great! Let's continue.
### Why not Rewire
Another popular solution for this problem is to use [rewire][rewire], which may _rewire_ module from inside.
```js
const file = rewire('./file');
file.__set__('fs', sinon.stub())
```
Actually, the code above would not work, by design, as long as is not compatible with `const` declaration we used to define `fs`.
Yes, you may use `let` or `var`, but is it a good solution?
From another point of view, tests would require changing `wasRoot` value, which is derived from `isRoot` and should be the same, while
you will have to mock them separately. This library is for _monkey-patching_, not for mocking.

### Mock-fs
It might be obvious, that [mock-fs][mock-fs] (which _mocks fs_) would help here - if `fs` is not real, then your files
would not been deleted during tests. Unfortunately it would not work again.
- firstly - how to control `isRoot` and `wasRoot`?
- secondary - some testing systems, like [ava][ava], or other modules, you dont have intention to mock, might require
a _real_ `fs`. So there is no way to use such _global_ hooks.

### Overall
There is an even more obvious reason why non require-time mocking is a bad idea.
Sinon(`.stub(exports)`) or Rewire(internal rewire) are working __after file got required__ and initialized,
thus - could not be used to sandbox or isolate system under test. It's too late.

## Dependency Mocking
"Dependency Mocking" is about replacing one __module__ by another. Replace `fs` as a module, not a local variable.
There are many different libraries, and some of them are even not "require-time" mocking, and
all they are doing their job a bit differently. Roughly in two different ways:
- [proxyquire][proxyquire] and [rewiremock][rewiremock] would replace __direct__ dependencies
- [mock-require][mock-require] and [mockery][mockery] would replace __any__ dependency

Usually the first variant is better, as long as you are mocking, and might mock, only something you know - the
system under test and it's explicit dependencies, so everything is _scoped_.
The second variant is also very valuable, and you might need need from time to time.

## Example

Expand Down Expand Up @@ -46,12 +109,12 @@ module.exports = doesFileExist;
In order to isolate our `doesFileExist` module for testing, we will stub out `fs` and provide a fake implementation of `fs.existsSync`, where we have complete control of the behaviour.

```javascript
var proxyquire = require('proxyquire');
var sinon = require('sinon');
var assert = require('referee').assert;
const proxyquire = require('proxyquire').noPreserveCache();
const sinon = require('sinon');
const assert = require('referee').assert;

var doesFileExist; // the module to test
var existsSyncStub; // the fake method on the dependency
let doesFileExist; // the module to test
let existsSyncStub; // the fake method on the dependency

describe('example', function () {
beforeEach(function () {
Expand All @@ -66,19 +129,36 @@ describe('example', function () {
});

describe('when a path exists', function () {
beforeEach(function() {
it('should return `true`', function () {
existsSyncStub.returns(true); // set the return value that we want
var actual = doesFileExist('9d7af804-4719-4578-ba1d-5dd8a4dae89f');

assert.isTrue(existsSyncStub.calledWith('9d7af804-4719-4578-ba1d-5dd8a4dae89f'));
assert.isTrue(actual);
});

it('should return `true`', function () {
it('should return `false`', function () {
existsSyncStub.returns(false); // set the return value that we want
var actual = doesFileExist('9d7af804-4719-4578-ba1d-5dd8a4dae89f');

assert.isTrue(actual);
assert.isTrue(existsSyncStub.called);
assert.isFalse(actual);
});
});
});
```
Look at the example above - code coverage is 100%. We have tested all branches we need,
and even tested that `fs.existsSync` was called with a right arguments.

Dependency mocking is a powerful tool, but sometimes you have to extract some functionality
to another module to be able to mock it.


[IoC]: https://en.wikipedia.org/wiki/Inversion_of_control
[di]: https://en.wikipedia.org/wiki/Dependency_injection
[ava]: https://github.com/avajs/ava
[mock-fs]: https://github.com/tschaub/mock-fs
[rewire]: https://github.com/jhnns/rewire
[seams]: http://www.informit.com/articles/article.aspx?p=359417
[proxyquire]: https://github.com/thlorenz/proxyquire
[demo-proxyquire]: https://github.com/sinonjs/demo-proxyquire
Expand Down