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

Introduce a new approach to mocking modules in tests #1061

Merged
merged 7 commits into from
Apr 23, 2019

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Apr 11, 2019

This PR introduces a new approach to mocking imports in tests using a Babel plugin to replace proxyquire/proxyquireify.

Rationale

There are currently a number of pain points with mocking in our JS code. The main one is that because of the way proxyquire works, it can lead to problems that are pretty confusing to debug. It is also inefficient because the module under test gets re-evaluated every time proxyquire is called. The Babel plugin here transforms the code in a straightforward way which makes mocking work similarly to how patch does in Python, with fewer foot-guns and better performance.

A secondary issue is future-proofing. Proxyquire only understands ES5 directly and is tied to Browserify/Node. The Babel plugin added here understands ES6 import syntax directly and works regardless of environment. My intention is that if we use it across all of our projects, this will enable us to make other changes that simplify our tooling overall.

Aside from fixing these problems, there are some additional benefits:

  • With this plugin an error will be thrown at runtime you if you provide mocks which do not match what a module actually imports - for example if an unused mock is provided or a typo is made in the module name. This found several problems in the code I've converted so far.
  • Tests build and run a few seconds faster. Because this uses a Babel plugin, it can work with the already-parsed code, rather than having to re-parse and process the code a second time as proxyquire does.

To see what this plugin does to a module, you can run Babel locally (with this branch checked out, and having run yarn install):

npm install -g @babel/cli
babel --plugins mockable-imports <path to module you are testing>

Usage

Mocking ES6 (import foo from "bar") or Common JS (const foo = require("bar")) imports in tests with this plugin works as follows:

  • When this plugin is enabled, every module exports an $imports object with $mock and $restore methods
  • In a test suite's beforeEach or in the test itself, call $imports.$mock(mocks) where mocks is a mapping from import path to exports. $mock can be called multiple times so you can "layer" mocks - eg. have a default mock for a test suite and then override it in a specific test.
  • Add an afterEach to the test suite which calls $imports.$restore to undo mocks.

Drawbacks

There is one downside to this approach compared to Proxyquire, which also applies when we patch stuff in Python. Because mocking does not re-evaluate the module being tested, you can't use it to change the result of code that is executed when the module is first imported. For example if a module has:

import helper from './utils/helper';

export const aConstant = helper(someData);

export function usesHelper() {
  return helper(someOtherData);
}

You could mock helper in usesHelper but not the initialization of aConstant. Based on my experience converting existing uses of Proxyquire, and also our experience of unittest.mock in Python (which has the same limitation) my instinct is that this won't be a significant problem, but it will come up at some point. It might actually be a good thing by encouraging side-effect free modules.

PR notes

The first commit adds the plugin to the test infrastructure. The subsequent commits change existing uses of proxyquire to verify that it works. To keep the PR smaller, I haven't converted everything in this PR or removed proxyquire. That will come in follow-up PRs.

I'd suggest reading the notes above and then looking through commit by commit. I'm happy to split some of the later commits into separate PRs if that makes things easier for review.

This provides an alternative way to mock a module's dependencies that
fixes several issues with proxyquire and proxyquireify [1]. In
particular we have had issues in the past with:

- Third party modules that have global state (eg. angular) causing
  hard-to-debug errors when mocking
- An initial attempt to convert Common JS imports to ES 6 imports ran
  into problems with the transformed code breaking proxyquire

This also provides future-proofing as well in that it enables us to
change the module bundler or test runner without changing how mocking
works. It should also be much more efficient if we make more extensive
use of import mocking as AngularJS is gradually removed from the codebase.

[1] https://github.com/robertknight/babel-plugin-mockable-imports#how-does-this-plugin-differ-from-alternative-approaches
@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c6128ee). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1061   +/-   ##
=========================================
  Coverage          ?   92.03%           
=========================================
  Files             ?      154           
  Lines             ?     6077           
  Branches          ?      957           
=========================================
  Hits              ?     5593           
  Misses            ?      484           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6128ee...b8d64a4. Read the comment docs.

robertknight added a commit to hypothesis/h that referenced this pull request Apr 12, 2019
…plugin

This is mostly done for consistency with the client. It will also make
some future changes to the code (eg. adoption of ES imports or changes
to the bundling) easier if/when we decide to do that.

See hypothesis/client#1061 for background.
events,
};
afterEach(() => {
GroupListItemOutOfScope.$imports.$restore();

Choose a reason for hiding this comment

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

This is for cleaning up the mock objects between tests.

Choose a reason for hiding this comment

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

The docs note that Jest would do this automatically for us. This test runner looks pretty nice. Is that something you think we might move to in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. As I understand it, Jest currently runs only under Node and not in a real browser. That is OK for many web apps where a fake browser environment like JSDOM is good enough for running tests. For the client I'm not sure if it will be adequate.

Copy link

@hmstepanek hmstepanek left a comment

Choose a reason for hiding this comment

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

This looks really good! I'm excited to see the code looking simpler and easier to follow now! The similarity between this and python import mocking is a plus IMO. I notice only some of the modules that use proxyquire have been converted over, you've noted that in this PR, and intend to continue to make follow up PRs to convert the rest. I just wanna make sure those don't get lost and we actually follow through with that but this LGTM!

@robertknight
Copy link
Member Author

I just wanna make sure those don't get lost and we actually follow through

Yes, I'll post the remainder of the PRs to finish this in the next day or two.

@robertknight robertknight merged commit 4e975cf into master Apr 23, 2019
@robertknight robertknight deleted the mockable-imports branch April 23, 2019 05:31
robertknight added a commit to hypothesis/h that referenced this pull request May 2, 2019
…plugin

This is mostly done for consistency with the client. It will also make
some future changes to the code (eg. adoption of ES imports or changes
to the bundling) easier if/when we decide to do that.

See hypothesis/client#1061 for background.
robertknight added a commit to hypothesis/h that referenced this pull request May 2, 2019
…plugin

This is mostly done for consistency with the client. It will also make
some future changes to the code (eg. adoption of ES imports or changes
to the bundling) easier if/when we decide to do that.

See hypothesis/client#1061 for background.
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

Successfully merging this pull request may close these issues.

2 participants