More tests - #41
Conversation
Karma unit tests are run in a browser, not in node.
Statically list all modules in this library, gather the injectables that
are registered (all directives, services, constants, etc.), then test
that each one can be injected without error so as to uncover usage of
injetables that aren't given access to via the module dependencies.
Below are some examples of fixes made for errors found by this test. The
stack trace is removed for readability.
A fix has been made to auth-storage-factory as the following was found:
```
Dependencies for module angular-token-auth.auth-storage should not error when injected FAILED
Expected angular-token-auth.auth-storage to have injectable factory authStorageFactory but it threw Error: [$injector:unpr] Unknown provider: authModuleSettingsProvider <- authModuleSettings <- authStorageFactory
<stack trace>...
Dependencies for module angular-token-auth.auth should not error when injected FAILED
Expected angular-token-auth.auth to have injectable factory authFactory but it threw Error: [$injector:unpr] Unknown provider: authModuleSettingsProvider <- authModuleSettings <- authStorageFactory <- authFactory
<stack trace>...
```
Fixing `authStorageFactory` also fixed the error in `authFactory` because
the latter depended on the former.
The module dependencies fix in d82266f
with this test would have erred like so:
```
Dependencies for module angular-token-auth.auth-login-form should not error when injected FAILED
Expected angular-token-auth.auth-login-form to have injectable directive loginForm but it threw Error: [$injector:unpr] Unknown provider: AuthLoginFormFactoryProvider <- AuthLoginFormFactory <- authLoginFormDirectiveFactory <- loginFormDirective
<stack trace>...
Dependencies for module angular-token-auth.auth-login-form-directive-factory should not error when injected FAILED
Expected angular-token-auth.auth-login-form-directive-factory to have injectable factory authLoginFormDirectiveFactory but it threw Error: [$injector:unpr] Unknown provider: AuthLoginFormFactoryProvider <- AuthLoginFormFactory <- authLoginFormDirectiveFactory
<stack trace>...
Dependencies for module angular-token-auth.auth-login-form-factory should not error when injected FAILED
Expected angular-token-auth.auth-login-form-factory to have injectable factory AuthLoginFormFactory but it threw Error: [$injector:unpr] Unknown provider: authActionsFactoryProvider <- authActionsFactory <- AuthLoginFormFactory
<stack trace>...
```
Avoid missing new modules or module renames by overloading angular.module to store all registered module names as `angular.registeredModules` array and loop over that to create the test suites.
They can't have dependencies so there's no point testing them for missing ones.
To ensure the other tests aren't false positives. Also improve the matchers code for readability.
| @@ -0,0 +1,167 @@ | |||
| (function () { | |||
There was a problem hiding this comment.
@maxpeterson This is the dependencies test I mentioned to you
There was a problem hiding this comment.
It's a bit funny that you need these test because of the pattern of putting everything into a separate module, so it's kind of solving a problem we have created ourselves ;)
There was a problem hiding this comment.
I see what you mean, but the core of the problem is the separation between module dependencies and the injectables they register. Though that problem is compounded by separate files (the issue with most Java code: one file per thing, it takes a long time to find what you want).
E.g. you can have a angular.module('bar', []) with module.factory('foo', ...), so you have to depend on the module bar to allow injecting the factory foo. There's no explicit connection there, only that which you create via naming.
The only advantage I see to Angular 1's module system is avoiding unnecessarily running config and run blocks that don't end up getting used. All the code is parsed anyway, so it's not really much of a saving.
The latest js frameworks use import/export so that simply importing something makes it available. That's way simpler and more descriptive.
There was a problem hiding this comment.
What I mean is that if every injectable in this package would just be under one module angular-token-auth then testing the availability wouldn't be necessary at all. It doesn't allow to specify which modules you do or don't want when using this package, but on the other hand maybe all injectables that require each other should be just under one module?
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 48.18% 74.09% +25.9%
==========================================
Files 18 18
Lines 193 193
==========================================
+ Hits 93 143 +50
+ Misses 100 50 -50
Continue to review full report at Codecov.
|
| }); | ||
| it('should redirect to LOGIN_REDIRECT_URL', function () { | ||
| expect(this.$location.url).not.toHaveBeenCalled(); | ||
| inject(function (AuthLoginFormFactory) { // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
It's an automatic login if an auth token has been saved: revisit the app and you'll be automatically logged back in.
When the AuthLoginFormFactory is first injected, the definition function is run and, as usual, Angular stores the return value as a singleton and doesn't run the definition again (thankfully). The "run once" bit is crucial here.
In the definition is this bit of code that runs at runtime. It means as soon as a thing injects AuthLoginFormFactory, the code checks for an auth token and redirects if it's there. Without "run once", the app would redirect multiple times and be quite buggy.
A better solution would be to have it as a method and call it in a run block, for the same effect. However that may be more difficult to test: not a reason to not do it, just a precaution. Or it could be left up to the project to call it.
|
|
||
| it('should not error when injected', function () { | ||
| var moduleDefinition = angular.module(moduleName); | ||
| moduleDefinition._invokeQueue.forEach(function (providerArgs) { |
There was a problem hiding this comment.
Should it be separated tests? It feels like this test is doing a lot.
There was a problem hiding this comment.
It is doing a lot, yes. There's an individual suite for every module: the code here is creating multiple tests from one loop function, which allows us not only to be DRY, but also to ensure no module is missed. A static list of tests would mean any new module would not be automatically included.
I like the automatic inclusion, and it helps the meaning of this test: to let the dev know when module dependencies haven't been listed, so the src changes in d82266f aren't needed down the line, and tests don't fail confusingly (the error output for missing module dependencies is often truncated so much as to be useless).
4 participants
@incuna/js Please merge, ta!