Skip to content
This repository has been archived by the owner on Aug 13, 2020. It is now read-only.

Strange promise behavior. #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Strange promise behavior. #100

wants to merge 1 commit into from

Conversation

cthrax
Copy link

@cthrax cthrax commented Apr 8, 2016

While using ng-describe, I ran into an issue resolving promises across tests. If the promise to be resolved comes from another service within the service under test, that promise will only resolve once. Despite new instances of everything being created for each test.

I've created a set of tests that demonstrate the issue here.

Additionally, I have pulled these same tests out of ng-describe and they run as expected. This had worked fine until upgrading to angular 1.5, in case that could be a hint, but I'm not sure that's the cause.

I'm really not sure where to begin debugging this, otherwise I would look a bit more at it myself. If you had some suggestions, I'd be happy to attempt a PR with a fix.

@pitieu
Copy link

pitieu commented Aug 15, 2016

The problem is that the way you create the service it generates a singleton. Even though you did new S1() and called it with ParentService, it will always return the same instance. To solve it you need to generate a new instance on demand.

The second problem is the $q for some reason he could not resolve second case(also singleton for some reason?). I tried with another library Q from Kriskowal and it solved the problem.

I'm not sure why $q is not working correctly. $q.defer() should create a new instance of it which makes me question also why it's not working.

// insert Q library from https://github.com/kriskowal/q
angular.module('ServiceModule', [])
    .factory('s1', function () {
      var S1 = function () {
        this.deferred = Q.defer();
        this.rand = Math.random();
      };

      S1.prototype.resolveWith = function (value) {
        this.deferred.resolve(value);
      };

      S1.prototype.getPromise = function () {
        return this.deferred.promise;
      };
      return {
        getInstance: function () {
          return new S1();
        }
      };
    })
    .factory('ParentService', function (s1) {
      var ParentService = function () {
        this.S1 = s1.getInstance();
        this.rand = this.S1.rand;
      };

      ParentService.prototype.resolveService = function () {
        this.S1.getPromise()
        .then(function (done) {
          done();
        });
      };

      return ParentService;
    });

/* global ngDescribe, it, xit */
ngDescribe({
  name: 'promises resolve across tests',
  modules: 'ServiceModule',
  tests: function ($rootScope, s1, ParentService) {
    var parent;
    beforeEach(function () {
      parent = new ParentService();
    });
    it('it can resolve promise once', function (done) {
      parent.resolveService();
      parent.S1.resolveWith(done);
      $rootScope.$apply();
      console.log(parent.rand);// different random value
    });
    // If both tests are run, it fails, either of them individually will pass
    it('it can resolve promise twice', function (done) {
      parent.resolveService();
      parent.S1.resolveWith(done);
      $rootScope.$apply();
      console.log(parent.rand);// different random value
    });
  }
});

@cthrax
Copy link
Author

cthrax commented Aug 15, 2016

Thanks for responding!

I think there is a mismatch between my expectations and the actual implementation.

My expectations are that the function for "tests" is injected beforeEach test, so that each tests runs independently. The singleton service was actually intended here because it's testing code that has multiple consumers of the promise and a singleton is the only way to pass state like this.

However, with the tests, it appears that only one injection is occurring for the suite of tests, which is why the singleton becomes an issue (multiple resolves on a deferred object don't make sense).

That's a fine implementation choice, I'm sure it made some things easier, but it didn't match what I expected, so sorry for the false report.

Cheers!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants