Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrap firebase auth with test waiters #330

Closed

Conversation

gabrielgrant
Copy link
Contributor

fixes #312

I've added the wrapper to the OAuth popup login path too, though it's not totally clear how this interacts with an ember acceptance test: running the tests in the browser seems seems to work with facebook login (so long as I've already authenticated the browser with the app manually) but I haven't thoroughly tested with other OAuth providers

@tstirrat
Copy link
Contributor

tstirrat commented Dec 1, 2015

Thanks for this, I like the idea, but think we can remove a lot of the duplication.

I wonder if we can clean this up by adding a node style to promise wrapper that also inserts the test waiters:

var authCalls = 0;

// ...
promisify: function(ref, methodName, args) {
  return new Ember.RSVP.Promise((resolve, reject) => {
    if (Ember.testing) {
      Ember.Test.registerWaiter(() => authCalls === 0); 
    }
    authCalls++;
    var callback = function(error, authData) {
      authCalls--;
      if (error) { 
        reject(error);
      } else {
        resolve(authData);
      }
    };

    args.push(callback);
    ref[methodName].apply(args)
  });
}

// called like so
return this.promisify(this.get('firebase'), 'authWithCustomToken', [options.token]);

@gabrielgrant
Copy link
Contributor Author

Hey, @tstirrat sorry for the delay in getting back to this. While my quick hack got this working for me, you're right that a promise wrapper is probably a cleaner way to go to avoid the duplication.

@tstirrat
Copy link
Contributor

I'm currently in the process of adding waiters to all the model requests. So I can cover this auth stuff in that PR, too.

More than happy for you to fix up this PR in the meantime, just let me know, either way.

@tstirrat tstirrat mentioned this pull request Dec 18, 2015
2 tasks
@tstirrat
Copy link
Contributor

Closing this in favor of #344 where I will use a common mixin

@tstirrat tstirrat closed this Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error Running Acceptance Tests
2 participants