Eslint-plugin-import: Add Jest .mock() / .unmock() / .disableAutomock() / .enableAutomock() as exceptions to imports-first?

[elliottsj]

Currently, the imports-first rule is triggered when Jest statements like jest.unmock() come before any import statements, e.g.

jest.unmock('../storage');

import { LOAD } from 'redux-storage';
import storage from '../storage';

describe('storage', () => {
  it('should initially have isStorageLoaded = false', () => {
    expect(storage(undefined, { type: 'FOO' })).toEqual({ isStorageLoaded: false });
  });

  it('should set isStorageLoaded = true when a LOAD is dispatched', () => {
    expect(storage(undefined, { type: LOAD })).toEqual({ isStorageLoaded: true });
  });
});

Error:

storage-test.js
  3:1  error  Import in body of module; reorder to top  import/imports-first
  4:1  error  Import in body of module; reorder to top  import/imports-first

As described in the Jest docs:

When using babel-jest, calls to unmock will automatically be hoisted to the top of the code block.

So jest.unmock() calls will ultimately be hoisted above import statements anyway, which makes sense since the modules you want to unmock must be specified before importing them.

---

Does it make sense for the imports-first rule to make exceptions for these Jest calls? Or maybe make the rule configurable to allow specific expressions to appear above imports?

[benmosher]

I think it probably falls outside the scope of the rule to allow certain expressions, though it does allow directives so it's not unprecedented.

Also, if babel-jest handles it, sounds like you could put the unmock calls anywhere (including after your imports) and it should still work.

So why put them before the imports? Notably, imports are also technically hoisted, so there is no semantic difference to putting them first.

[elliottsj]

It works if I put the unmock calls after the imports, but I think it makes a bit less sense to someone reading the tests. To me, it makes more sense to read it as

// First tell Jest not to mock '../foo'
jest.unmock('../foo');
// Then import it
import foo from '../foo';

rather than

// Import '../foo'
import foo from '../foo';
// Then tell Jest not to mock it, even though it seems we already imported it?
jest.unmock('../foo');

If the reader isn't aware that babel-jest is going to hoist the jest.unmock above the import, it can be a bit confusing.

[benmosher]

Sure. I like it more the second way, personally, but it feels a bit arbitrary. (also, I'm not writing Jest tests. which is to say: my opinion is meaningless. 😄)

I'm inclined to reject this on grounds of keeping it simple, especially because there is already tool support for your case writing all the imports first, and since imports are hoisted anyway there is never a semantic reason to write anything else first (barring the aforementioned already-supported directives).

@jfmengels, second opinion?

[jfmengels]

I agree with @benmosher. I agree that having unmock before may make a bit more sense (arguably), but if possible, I'd like to keep this rule simple. Especially as in this case, it doesn't make any difference in the output.

If it helps, you can think of the import section being a table of content of the dependencies, and having it at the very top of every file helps you find them easily. Having this consistency is one of the reasons IMO of having such a rule. (Granted, a one line statement won't hinder this)

Then again, this is mostly just to avoid yet another specific exceptions to a rule of this plugin :confused:

PS: I also have 0 experience with Jest

[benmosher]

Closing in the name of simplicity. Thanks for your issue, btw! Even if we don't implement it, it's great to get feedback. 😄

[elliottsj]

Alright, sounds good 😄

[cpojer]

Just to provide some context: Jest mocks on the module boundaries. Personally I would prefer people to use require instead of imports in tests but the community has decided they love imports, even for tests, so there was little reason in fighting it.

The only solution that made sense was to hoist the mock directives above imports because when a module is required, Jest needs to know whether it should require a mock or the real module. This works surprisingly well but of course kind of goes against ES. Since this is in tests and for the foreseeable future we will continue to transform at least test files in node, this will continue to work well.

Personally I would recommend people to put these Jest calls above import statements and it would be nice if a plugin such as this one could support this use case. Can we add a whitelist option to the import plugin where we could specify jest\.(mock|unmock|disableAutomock|enableAutomock) or similar which this plugin will use to ignore statements above imports? That would be awesome and not very opinionated.

[astorije]

_(Sorry to bump an old thread)_

I understand the argument above that some positioning of jest.unmock() might be a personal preference, but in some cases, it simply won't work otherwise. For example, consider the module AuthManager makes a call to a side-effect logout function that you want to mock (and where refactoring is not an option ahah):

// This needs to be run before importing `AuthManager`
jest.mock('../src/utils/logout', () => ({ logout: jest.fn() }));

import { AuthManager } from '../src/AuthManager';

import/order will invert those statements, which will now fail the tests. For the moment, I will locally disable it, but fixing the test also conflicts with import/order so now I have a bunch of things to silence 😅

So I know that this is frowned upon, but I would love a whitelist as well.

---

EDIT:

It turns out it was a little more convoluted than that. A better example of what I was encountering is the following:

const logout = jest.fn();

// This needs to be run before importing `AuthManager`
jest.mock('../src/utils/logout', () => ({ logout }));

import { AuthManager } from '../src/AuthManager';

/* ... */

expect(logout).toHaveBeenCalled();

With that example, I would have to keep import after jest.mock. However, a small change to my test fixed it:

import { AuthManager } from '../src/AuthManager';
import { logout } from '../src/utils/logout';

jest.mock('../src/utils/logout', () => ({ logout }));

/* ... */

expect(logout).toHaveBeenCalled();

See https://jestjs.io/docs/en/manual-mocks#using-with-es-module-imports.
This passes both the tests and the linter. I am not 100% sure why the first example doesn't hoist the mock call at the top, but I'm happy with the alternative.