Redirect require calls in tests to support mocks

[juliandescottes]

Proposal

While working on remote-debugging for the new about:debugging, we wanted to test UI components related to USB debugging with mochitests. Since we obviously can't have a USB device available to run mochitests, we decided to isolate the modules that have to interact with USB devices and to try to mock them during the test.

To enable mocking, we think that redirecting require calls is a good option: when calling require("path/to/module"); instead of returning the real module, it would return an object provided by the test. The added value is that it doesn't force the implementation to be re-architected just to be testable. Compared to other approaches we have in DevTools (exposing test-only methods for instance), this should make it easier to test things which are not easily doable in test environments without impacting the implementation.

In Bug 1497917, we have used this approach and you can have a look at the following phabricator patch to see how this may look like (was very hacky at this stage, the goal was just to have a proof of concept).

The goal of the RFC is to:

(1) know if others in the DevTools community already faced a similar situation and have requirements or expectations for this kind of feature

(2) check if DevTools peers are comfortable to modify our loaders with some test-only code

(3) maybe there is a way to achieve this with the current loaders (knowing here we need to redirect require calls made in a BrowserLoader)? Couldn't make it work but the Loader code is hard to get into.

Implementation suggestion

Technically the proposal would be to populate a globally accessible object from test code, with an API such as:

  const mock = { someAPI: () => { return 0; } };
  setMockedModule(mock, 
    "resource://devtools/client/aboutdebugging-new/src/modules/some-module");

The test would also cleanup the mocks after they are no longer needed

  removeMockedModule(
    "resource://devtools/client/aboutdebugging-new/src/modules/some-module");

The loaders should be modified to check for mock entries. We should update:

• browser-loader.js (in requireHook)

  requireHook: (id, require) => {
  
  // [...]
  
  const uri = require.resolve(id);
  
  if (flags.testing && hasMockedModule(uri)) {
  return getMockedModule(uri);
  }
  
  // [...]
  }

• base-loader.js (in _require)

  function _require(id) {
  let { uri, requirement } = getRequirements(id);
  
  if (flags.testing && hasMockedModule(uri)) {
  return getMockedModule(uri);
  }
  
  // [...]
  }

Technically all the get/set/has/removeMockedModule() would be available on a global object that can be shared by all the loaders.

If we agree on a proposal here, I will be happy to take care of the implementation.

[nchevobbe]

I just wanted to point that In the webconsole mocha tests, we are using the require-hacker lib to mock the files we want: https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/devtools/client/webconsole/test/mocha-test-setup.js#39-95

[MikeRatcliffe]

We have talked in the past about doing this and have always agreed that there are times it could be really useful... no objections from me.

[sole]

I wonder what would be better... to use require-hacker on more panels (after some documentation to make it known)... or to use the implementation Julian described throughout 🤔

One gets us closer to what other people in the Web are doing, the other one gives us full control.

[juliandescottes]

I wonder what would be better... to use require-hacker on more panels

I think require-hacker only works in a unit test context, can't work with mochitests. If we can have mocha-tests running on try soon-ish why not, but otherwise I'd still petition for a solution compatible with mochitests.

[julienw]

As a piece of advice, this is a mock style that Jest supports, and so this seems like a good idea to support in our tests too. Inside the mocks themselves please use sinon (as it's available) to generate mock functions instead of reinventing the wheel :)

[jasonLaster]

the debugger will be landing jest tests in the near future. That could be good too.

[juliandescottes]

Thanks for the feedback so far! Looks like there are no strong opinions against doing this, but several of you mentioned unit testing solutions that support mocking require. If we had a decent component unit testing solution, that would probably be a good alternative. In the end I prefer to avoid modifying the loader with code that might only be used by the new about:debugging.

@nchevobbe @jasonLaster is one of your unit testing solution close to being run on continuous integration (tier 1 preferably)?

[nchevobbe]

There's nothing being worked on for the console at the moment.

[juliandescottes]

We discussed about this RFC yesterday in the RFC review meeting. While there are no clear objections to modify the loader, it seems that most of the other panel have addressed similar issues by using unit tests and would not be relying on this new feature.

It is not clear if unit tests would give us the same coverage as what are achieving right now with mocks and mochitests. Running those unit tests in CI is not something that will happen anytime soon (6months - 1year maybe), so it won't be a good fit for our work on aboutdebugging-new anyway. Most of our tests will be already written by that time, and I doubt we'd like to invest in rewriting everything after the fact.

As the logger of the RFC, I prefer to leave it open for one more week, see if anyone outside of aboutdebugging-new would be interested in using it. If not we'll have to decide between modifying the loader for something that we might only use in aboutdebugging-new, or keep our current workarounds in the aboutdebugging-new codebase.

[gregtatum]

In my opinion, increasing our ability to mock out resources in tests will increase our ability to write higher quality tests with fewer hacks and work arounds. I think this should definitely go forward even if aboutdebugging-new is the only current consumer.

[juliandescottes]

Stumbled upon the following helper today: https://searchfox.org/mozilla-central/source/testing/modules/MockRegistrar.jsm

Looks like there is an existing helper to mock components. I don't think we can easily modify it to do what we need, but it's still interesting to see other test suites had a similar need.

[ochameau]

Sounds good to me. I'm not sure Jest is an answer to your need. It would limit mocking only to React component testing, right? Also, before introducing a new test harness (we already have xpcshell, mochitest plain [just two], mochitest chrome and mochitest browser), we should think twice about that. But again, it sounds like another discussion.

Otherwise, the final implementation may be subject to discussion as it might be more complex to mock modules that are already loaded by the the test head files. It will most likely be easier to implement such feature in browser loader, if that's enough, as test helpers won't be instantiating them. If there is a significant traction into using Jest, we may want to see if we can come up with an API/behavior that is similar?

[julienw]

As another quick note, I didn't want to suggest that we should use Jest, I mentioned it to show that this capability seems to have an interest outside of our project too.

[juliandescottes]

Thanks for the feedback! The RFC has been accepted and will be implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1510545