Closed THemming closed 2 years ago
Hey @THemming ! Thnx for the PR, it is a nice one and I understand what you are trying to achieve. Having said this it will take a bit of time before this one can get into the repo since I'm planning a bigger rewrite of the modules loading algorithm (see #21). I don't want to add more substantial code chunks before this rewrite. So I will have to ask you for a bit of patience.
On a separate thought - while I understand the "mock on a file / package level" principle, personally I find that using a DI container and mocking on a class level to be a more elegant solution closer to the spirit of unit testing. But this might be just a personal preference so feel free to ignore this remark.
Thanks for the feedback. If I need to I'll publish a fork on NPM until this functionality gets merged.
It'd be great if during the refactoring you could take into account the integration of loader plugins for functionality such as this. Being able to bypass the cached modules and also to create a new context in which to load modules.
@THemming you can track WIP in the refactoring here: https://github.com/karma-runner/karma-commonjs/issues/21
and this is the first PR moving in this direction: https://github.com/karma-runner/karma-commonjs/pull/22
As you can see it introduced a concept of several candidate files resolving a given require and IMO it could be use to inject mock mappings.
Is karma-commonjs really the right place for this functionality? This seems like it adds complexity (and encourages poor testing practice). If you really want mocking in your tests, couldn't you use a mocking library like Sinon or similar?
@jamesshore I'm also not big "fan" of "mocking on the file level" as personally I find that DI + dynamic nature of JS gives me better working / testing env. @THemming would be awesome if you could elaborate on your exact use-case just to makes sure that we are not missing anything here.
I think karma-commonjs is the right place for this as injecting alternative implementations of modules has to be done by the module loader, or a plugin thereof. A mocking framework would still be used to actually create the mocks or spies.
Of course there are different schools of thought on how to load dependencies and I find a mix of passing dependencies through to constructors or setter methods and being able to inject mocked modules an elegant solution. One nice aspect is that you don't need to introduce yet another concept into your code, a separate DI system, when the commonjs module system is already being used to load dependencies.
I think the Jest testing framework sums it up nicely, see Jest CommonJS Testing. Another example of require mocking is proxyquire.
As an example if you had a dependency upon Bootbox.js and were to load it into your module using the require() mechanism you could easily mock this when running tests. You wouldn't particularly want to pass this sort of 'singleton' object in through a constructor and having to load it in through some other DI system would feel like a bit of an overhead.
If you had a module that depends upon an instance of a rest client, for example, then I wouldn't propose exposing that instance of the rest client as a module in its own right. That sort of instantiated object could be passed in as a constructor argument.
Is there a way we can make karma-commonjs work with Jest (and proxyquire, and whatever might come along in the future) rather than doing our own implementation? Or is that what this PR does?
This pull request adds in a feature to allow easy mocking of modules. I've included unit tests and a documentation update that shows how this feature is used.