Unit Test Coverage on Angular Services and Providers

[kanhirun]

Summary

• Provides full unit test coverage for all angular services—namely, zendeskWidgetSettings, and ZendeskWidgetProvider (4d95fcc) (See Figure 1 below)
• Reorganizes test directory into testing layers, integration/ and unit/ (see Figure 2 below)

Figure 1. Unit Test Coverage Report

Missing 6.25% is due to Self-Executing Anonymous Function [LOC]

----------------------------------|----------|----------|----------|----------|----------------|
File                              |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------------------------------|----------|----------|----------|----------|----------------|
 angular-zendesk-widget/          |      100 |      100 |      100 |      100 |                |
  zendeskWidget.module.js         |      100 |      100 |      100 |      100 |                |
 angular-zendesk-widget/services/ |    92.86 |      100 |     87.5 |    92.86 |                |
  zendeskWidgetProvider.js        |    90.91 |      100 |    85.71 |    90.91 |             29 |
  zendeskWidgetSettings.js        |      100 |      100 |      100 |      100 |                |
----------------------------------|----------|----------|----------|----------|----------------|
All files                         |    93.75 |      100 |    88.89 |    93.75 |                |
----------------------------------|----------|----------|----------|----------|----------------|

Figure 2. The test/ is reorganized to expose two distinct layers

test
├── integration/               <— tests angular's bootstrap phase
│   └── zendeskWidgetSpec.js
└── unit/                      <— tests angular's configuration, and run phase
    └── services/
        ├── zendeskWidgetProviderSpec.js
        └── zendeskWidgetSettingsSpec.js

[kanhirun]

@bsouthga Ship is ready for sail. Ready for review.

[bsouthga]

@kanhirun this looks great! thanks for adding code coverage + a few more tests. 👏

There is only one issue I see to take a look at before merging this in -- when I use this branch with our current app, it I get the

angular-zendesk-widget.js:15 Uncaught Error: Missing accountUrl. Please set in app config via ZendeskWidgetProvider

Error, as it looks like the new order of the concatenated files causes the run statement to occur before the config registration.

Maybe take a look at that? It also might make sense to add a real index.html test file with an example of injecting the plugin to a fake angular module.

[kanhirun]

@bsouthga I'm not sure what the proper naming is for angular files. Could I request for a review on 1bb64e1a?

[bsouthga]

@kanhirun the filenames look good to me, don't yet have a nailed down standard.

[kanhirun]

@bsouthga I made changes based on your feedback and tested it end-to-end with the angular-seed app. Thanks again for bringing this to my attention.

The issue wasn't just that the run statement occurred before config, but that both zendeskWidgetSettings and ZendeskWidgetProvider share settings in common, and it needs to remain shared even when the pieces separate into their own files—and not with their own copy of settings. The task became, how can these two components be separated into their own files while still sharing the same data?

Ultimately, I decided to just merge the two back into the same file. (After all, my testing requirement was just to keep the run statement by itself.)

If you want to take a different approach, I'll lean on your expertise here.

Here are some other solutions:

1. Define a global variable which both components have access to. 👎
2. Have the two components pass data in the "angular" way. I looked into this, but it seems to be complicated by several details that I will omit here.
3. Finally, we can probably configure concatenation to wrap all the files into one self-executing anonymous function. But here, we would break encapsulation across components.

Either way, this is ready for another review (and to be re-tested on your end)

[bsouthga]

Awesome! looks good to me -- I think keeping the two in the same file is fine for now. If we did want to move them to separate files, we could try injecting the settings into the provider like this:

  angular.module('zendeskWidget')
    .provider('ZendeskWidget', ['zendeskWidgetSettings', function(settings) {
     //... do stuff
    });

Overall great work 👍 -- appreciate your thoroughness and desire for implementing refactoring best practices!

[kanhirun]

@bsouthga Great! I'll leave it to you to press the big ✅ button then.

EDIT: To my understanding, zendeskWidgetSettings can't exist as a dependency because angular loads providers first before loading its services (i.e. Value, Factory). Indeed if you tried doing so, you'll get Unknown provider: zendeskWidgetSettingsProvider.

[bsouthga]

Your totally right, my mistake -- I think keeping the two in the same file is fine for now. Merged!