evangalen / ng-improved-testing

Improves AngularJS testing
MIT License
21 stars 4 forks source link

$q.tick doesn't work for multiple promises #7

Closed ocombe closed 9 years ago

ocombe commented 9 years ago

Hi,

We talked on twitter the other day, and you were wondering why I needed a setInterval for my tests that would trigger a digest every 10ms. The reason is: when I use multiple promises, one digest isn't enough to trigger all promises, just the first one. Also since it uses async storage (indexedDB for example), you don't know when the promise will actually resolve (I'm not sure if that's relevant but maybe it is).

I tried to use $q.tick from your lib to solve this problem, but I had to use a setInterval again because one $q.tick wasn't enough. I made a small repository with 2 simple tests so that you could see by yourself: https://github.com/ocombe/q-tick-tests

The first test works as expected with a setInterval, the second one doesn't with just $q.tick. Do you have any idea how I could do that without the setInterval ?

evangalen commented 9 years ago

Hi,

Thank you for submitting this issue and for making a small Jasmine spec along with it. I now exactly understand why it's now working :-)

Using $q.tick() will not solve your issue since $q.tick() is nothing more than a $rootScope.$digest() specifically targetted at $q and without the side-effects of triggering a real digest (i.e. causing $watch-es of a controller being testing to be triggered). For a future version of ngImprovedTesting I'm even planning to disable all registered $watch-es registered by a controller (when using a particular API) unless you specifically enable it (i.e. identified by the string expression or name of a watch function). Currently I'm thinking about a fixture-like solution like this:

it('...', testController('nameOfARegisteredController')
    .withWatch('aWatchedPropertyName')
    .do(function() {
        // ...
    });

Back to the your actual issue :-) Your problem is that localForage uses their own (non-AngularJS) promise which are wrapped in AngularJS promises. The problem is that your tests don't know (and shouldn't know) when the "then" method of the (Mozilla) localforage promise will be triggered. But what if we would use add an 'after returning' aspect to each method of "$localForage._localforage". The aspect could add logic to each method of "$localForage._localforage" to check if the returned result is an object with a "then" method. If this is the case than the "then" method logic also needs to be altered so that the "onFulfillment" and "onRejection" callbacks (of the Mozilla localForage Promise) are wrapped to do q.tick() after its actual execution.

You can use the following code the apply the 'aspect logic':

var originalLocalforageMethods = null;

beforeEach(function() {
  originalLocalforageMethods = {};

  angular.forEach($localForage._localforage, function(propertyValue, propertyName) {
    if (angular.isFunction(propertyValue)) {
      var originalMethod = propertyValue;

      originalLocalforageMethods[propertyName] = originalMethod;

      $localForage._localforage[propertyName] = function() {
        var result = originalMethod.apply(this, arguments);

        if (angular.isObject(result) && angular.isFunction(result.then)) {
          var originalThenMethod = result.then;

          result.then = function(onFulfillment, onRejection) {
            var modifiedOnFulfillment = undefined;
            if (onFulfillment) {
              modifiedOnFulfillment = function() {
                var result = onFulfillment.apply(this, arguments);
                $q.tick();
                return result;
              };
            }

            var modifiedOnRejection = undefined;
            if (onRejection) {
              modifiedOnRejection = function() {
                var result = onFulfillment.apply(this, arguments);
                $q.tick();
                return result;
              };
            }

            return originalThenMethod.call(this, modifiedOnFulfillment, modifiedOnRejection);
          };
        }

        return result;
      };
    }
  });
});

afterEach(function() {
  angular.forEach(originalLocalforageMethods, function(propertyValue, propertyName) {
    $localForage._localforage[propertyName] = propertyValue;
  });
});

The $q.tick() invocations can now be removed from the "setItem and getItem should work without setInterval" spec:

it('setItem and getItem should work without setInterval', function(done) {
  $localForage.setItem('myName', 'Olivier Combe').then(function(data) {
    expect(data).toEqual('Olivier Combe');

    $localForage.getItem('myName').then(function(data) {
      expect(data).toEqual('Olivier Combe');
      done();
    }, done);
  }, done);
});

In your particular case you probably don't really need the $q.tick() since de side-effects of using $rootScope.$digest (instead of $q.tick()) probably won't be an issue for you (since your tests most likely are really using any scope at all); but then again I would not mind if you would use $q.tick() ;-)

Let me know if you have additional questions.

ocombe commented 9 years ago

That's awesome, I'll test it tomorrow. And I may still use it for ocLazyLoad since I'll add tests with $scope when I have the time.