ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.88k stars 3.89k forks source link

Support dependency mocking for testing #5286

Closed aghassemi closed 6 years ago

aghassemi commented 8 years ago

We should integrate something like https://github.com/speedskater/babel-plugin-rewire so unit tests can mock sub dependencies.

babel-plugin-rewire requires babel 6+ however.

Integrating this should be fairly straight forward, we need to add "plugins": ["babel-plugin-rewire"], to .babelrc but we may want to add a test profile however to only use this plugin in test builds. Also need to see if this can work in combination with --compiled flag.

aghassemi commented 7 years ago

from @taymonbeal in https://github.com/ampproject/amphtml/issues/9163

Our (by which I mean A4A's) tests currently rely heavily on Sinon fakes of classes like Xhr. These tend to be fragile; a change to the internals of Fast Fetch (where it started calling fetchJson instead of fetch) meant rewriting all the tests to use new fakes. Also, some kinds of objects (like top-level functions and objects) are difficult or impossible to fake in this way. All of this makes test doubles really painful to work with.

Could it be feasible to have some kind of dependency-injection-esque setup instead? If a component depends on, say, Xhr, then in the real world it'd want to use the real Xhr service, but in tests it'd be desirable for the test author to be able to provide a test double, either from Sinon or a custom-written one for the test. As long as the test double conforms to a specified interface, the test should then be resistant to changes in the internal implementation details of the code under test.

aghassemi commented 7 years ago

/cc @rsimha-amp

dreamofabear commented 7 years ago

Re: #9163, @dvoytenko tackled a similar problem for globals like window via describes.js, which allows callers to use real or fake objects in fake-dom.js. It may suffice to have shared fake objects for commonly used services like Xhr + helper methods for easier stubbing.

IMO we can have better testability in dynamic languages without introducing dependency injection.

aghassemi commented 7 years ago

@choumx Globals are easier to mock since they are not import x from y. Totally agree that we don't want actual dependency injection in JS, but for testing we need some tooling help. For example if you are testing foo.js and foo.js does import bar from 'bar.js' and you want to mock bar, some tooling is required to rewrite the import to the mock version of bar.

taymonbeal commented 7 years ago

The other half of the problem is a need for stable interfaces that can be used as stub targets. E.g., if I'm faking Xhr in my test, then I ought to be able to write something that covers the "core" of what Xhr does (e.g., the part that makes the actual network request) and expect all the helper methods to still work. Otherwise you have the aforementioned problem where a refactor that changes fetchJson to fetch breaks all the tests.

It also sort of seems as though which substitutable services something depends on should also be part of its API, but if people don't want to do DI then maybe that doesn't make sense? I'm not sure.

Xhr in particular might have complex enough needs that it should get its own dedicated "fake server" test double (see #8020).

taymonbeal commented 7 years ago

Also, the idea of this being a Babel plugin makes me nervous. It seems desirable to be able to compile tests in Closure, to debug issues that manifest differently there, or to run the tests uncompiled once browsers support that. Could this happen at the AMP service level instead? What determines whether something is a service?

aghassemi commented 7 years ago

yeah whatever we do here needs to work with closure as well. We run compiled tests. Mocking a service (similar to mocking globals) is a simpler problem and may not need the generic ability to mock any deep dependency part. @dvoytenko

taymonbeal commented 7 years ago

To be clear, we do need the ability to mock some things that aren't currently implemented as services, and that in some cases are specific to a particular extension. (Example: The Fast Fetch signing service registry.) If making those things mockable requires them to be ported to the service framework, then we can do that, but I'd like to understand whether that's a good idea, hence my question about what makes a service.

dvoytenko commented 7 years ago

At this point we can mock all things except top-level function calls, right?

rsimha commented 6 years ago

Is this still something we want to do?

aghassemi commented 6 years ago

We can close this, the recent fetch-mock and service mocks have been very helpful and cover what I had in mind.

rsimha commented 6 years ago

SGTM.