Preliminary tests for $firebaseAuth #470

jamestalmage · 2014-11-18T17:18:42Z

I took a stab at it. Let me know what you think.

coveralls · 2014-11-18T17:29:07Z

Changes Unknown when pulling 5b462a1 on jamestalmage:firebase-auth-tests into * on firebase:master*.

jamestalmage · 2014-11-18T22:29:50Z

This only covers half the class (those methods deferring to _onLoginHandler).
I can flush it out to cover the rest of the class, but I wanted a project collaborator to approve before I spent more time.

jwngr · 2014-11-20T03:31:13Z

Thanks for the contribution! I realistically won't have a chance to play with/look into this PR for a couple days due to my schedule, but I will definitely get back to you as soon as I can. Keep up the awesome work!

jwngr · 2014-11-26T05:04:00Z

Hey @jamestalmage - apologies on the delayed review on this PR. Overall, I think it has some great stuff and urge you to continue pumping out more tests if you are up for it! My major comment would be that long-term, I think we will want to add the new authentication methods to MockFirebase and use those mocked methods in this AngularFire test suite. But I would be more than happy to merge in your test suite in the meantime if you want to keep going. This puts us 80% of the way to a final solution (if not more).

Commit @239e2de added an option options argument to authWithCustomToken. This caused a test failure. Change tests to reflect new API.

Delete extra whitespace

coveralls · 2014-11-28T23:01:03Z

Changes Unknown when pulling 6c80065 on jamestalmage:firebase-auth-tests into * on firebase:master*.

Tests for `$getAuth()`, `$unauth`, and `$onAuth`.

coveralls · 2014-11-28T23:43:59Z

Changes Unknown when pulling 9fb6bec on jamestalmage:firebase-auth-tests into * on firebase:master*.

coveralls · 2014-11-29T02:19:48Z

Changes Unknown when pulling a6b1a8a on jamestalmage:firebase-auth-tests into * on firebase:master*.

coveralls · 2014-11-29T02:50:39Z

Changes Unknown when pulling bb451be on jamestalmage:firebase-auth-tests into * on firebase:master*.

coveralls · 2014-11-29T02:59:25Z

Changes Unknown when pulling 4fdff7b on jamestalmage:firebase-auth-tests into * on firebase:master*.

jamestalmage · 2014-11-29T03:15:55Z

@jwngr @katowulf
100% coverage of FirebaseAuth.js.

@jwngr
Please review and merge at your convenience.

katowulf · 2014-12-01T15:37:50Z

tests/unit/FirebaseAuth.spec.js

+    inject(function(_$firebaseAuth_,_$timeout_){
+      $firebaseAuth = _$firebaseAuth_;
+      auth = $firebaseAuth(ref);
+      $timeout = _$timeout_;


Just intellectualizing here: I've started to prefer setting this.$timeout vs a global var $timeout and the _$timeout_ notation. Since we're in a beforeEach, this.$timeout is available in every scope and doesn't interfere with any other global variables set in the scope chain (a problem I've run into a few times in test units with commonly named variables, not specifically with $ services from angular).

I haven't made this suggestion for AngularFire specifically and we may keep the current convention for convenience; again, just intellectualizing about the cooking apparatus.

@katowulf The reason not to use that convention is that it's cumbersome to constantly be passing this around when you have tests with callbacks. I used to store test data on this and dropped the habit because constant self = this or binding was sloppier than the scope hoisting method.

Interesting. I haven't run into passing it around via callbacks yet. Great point.

katowulf · 2014-12-01T15:53:04Z

@jamestalmage great work! Some minor stylistic comments to consider; let me know when you've looked them over and we'll be good to merge. I'll leave the actual merging for @jwngr since this is his purview.

jamestalmage · 2014-12-01T19:13:28Z

@jwngr @katowulf
Made the changes, should be good to go.

katowulf · 2014-12-04T22:49:05Z

@jamestalmage we're just waiting on @jwngr to get some free time. Think of me like a copper alloy and Jacob like about three pounds of neodymium. In other words, it can be hard to get enough of him in one place at any given time.

jamestalmage · 2014-12-04T22:53:25Z

No problem. Recent merges required additional tests anyways.

coveralls · 2014-12-04T22:56:00Z

Coverage remained the same when pulling 0812dd3 on jamestalmage:firebase-auth-tests into b512baa on firebase:master.

It no longer fails, but there is still an issue.

…nded#496.

coveralls · 2014-12-04T23:11:34Z

Coverage increased (+0.01%) when pulling 88066b7 on jamestalmage:firebase-auth-tests into b512baa on firebase:master.

jwngr · 2014-12-05T05:30:17Z

tests/unit/FirebaseAuth.spec.js

+  describe('$authWithPassword',function(){
+    it('passes options and credentials object to underlying method',function(){
+      var options = {someOption:'a'};
+      var credentials = {username:'myname',password:'password'};


username -> email

jwngr · 2014-12-05T05:46:17Z

I'm not sure I really followed @katowulf's analogy, but thanks for your patience here @jamestalmage! I made a few final comments (nothing too big) and once you clean those up, this thing will get merged into master. We are probably going to cut a new release soon to get all the awesome changes you've introduced out to the public.

coveralls · 2014-12-05T06:23:45Z

Coverage remained the same when pulling e733955 on jamestalmage:firebase-auth-tests into b512baa on firebase:master.

coveralls · 2014-12-05T06:35:40Z

Coverage remained the same when pulling b5750b4 on jamestalmage:firebase-auth-tests into b512baa on firebase:master.

…mat. In all tests except those specifically testing deprecated behavior, use the correct argument format.

jwngr · 2014-12-05T06:40:53Z

You've got one last typo which is causing the tests to fail. Once that's fixed, this thing is gonna be merged!

jamestalmage · 2014-12-05T06:43:37Z

@jwngr
Typo fixed. Sorry should have ran tests before committing.
I also just changed all the test methods to use the non-deprecated calls (except those explicitly testing deprecated behavior)

coveralls · 2014-12-05T06:45:06Z

Coverage remained the same when pulling b985a4c on jamestalmage:firebase-auth-tests into b512baa on firebase:master.

jwngr · 2014-12-05T06:46:54Z

Merging in! This is some really fantastic work and it is extremely appreciated! You're killing it!

I also just sent a PR to get that typo fixed in our authWithOAuthToken() docs. That's what I get for rearranging lines...

Preliminary tests for $firebaseAuth

jamestalmage · 2014-12-05T06:48:27Z

Thank you for the kind words.

Preliminary tests for $firebaseAuth

5b462a1

jamestalmage added 3 commits November 28, 2014 17:39

Merge remote-tracking branch 'upstream/master' into firebase-auth-tests

cd4edbb

Fix tests after merge of @239e2de.

d240718

Commit @239e2de added an option options argument to authWithCustomToken. This caused a test failure. Change tests to reflect new API.

Fix code styling (auth-tests).

6c80065

Delete extra whitespace

More tests for FirebaseAuth.

9fb6bec

Tests for `$getAuth()`, `$unauth`, and `$onAuth`.

Tests for FirebaseAuth: $requireAuth / $waitForAuth

a6b1a8a

jamestalmage added 2 commits November 28, 2014 21:47

Tests for FirebaseAuth: user management functions

72d986a

remove istanbul ignore in FirebaseAuth

bb451be

Add test coverage for firebaseAuth

4fdff7b

katowulf reviewed Dec 1, 2014
View reviewed changes

auth-tests: minor style changes

2847a08

auth-tests: minor style changes.

d00919d

jamestalmage added 2 commits December 4, 2014 17:38

auth: Add tests for new deprecation logging and object arguments

d5c6685

Merge remote-tracking branch 'upstream/master' into firebase-auth-tests

0812dd3

jamestalmage added 2 commits December 4, 2014 17:56

Re-enable $createUser test.

79a71d9

It no longer fails, but there is still an issue.

Add test and resolution for FirebaseExtended#496. Closes FirebaseExte…

88066b7

…nded#496.

jamestalmage mentioned this pull request Dec 4, 2014

$createUser throws a ReferenceError #496

Closed

jwngr reviewed Dec 5, 2014
View reviewed changes

jwngr assigned jamestamplin and unassigned jamestamplin Dec 5, 2014

auth-tests: style changes. auth: return user object instead of just uid.

e733955

Separate tests for OAuth string token and credential objects.

b5750b4

Fix test failure. Switch usermanagement tests to use new argument for…

b985a4c

…mat. In all tests except those specifically testing deprecated behavior, use the correct argument format.

jwngr pushed a commit that referenced this pull request Dec 5, 2014

Merge pull request #470 from jamestalmage/firebase-auth-tests

18db305

Preliminary tests for $firebaseAuth

jwngr merged commit 18db305 into FirebaseExtended:master Dec 5, 2014

jamestalmage deleted the firebase-auth-tests branch December 5, 2014 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preliminary tests for $firebaseAuth #470

Preliminary tests for $firebaseAuth #470

jamestalmage commented Nov 18, 2014

coveralls commented Nov 18, 2014

jamestalmage commented Nov 18, 2014

jwngr commented Nov 20, 2014

jwngr commented Nov 26, 2014

coveralls commented Nov 28, 2014

coveralls commented Nov 28, 2014

coveralls commented Nov 29, 2014

coveralls commented Nov 29, 2014

coveralls commented Nov 29, 2014

jamestalmage commented Nov 29, 2014

katowulf Dec 1, 2014

bendrucker Dec 1, 2014

katowulf Dec 1, 2014

katowulf commented Dec 1, 2014

jamestalmage commented Dec 1, 2014

katowulf commented Dec 4, 2014

jamestalmage commented Dec 4, 2014

coveralls commented Dec 4, 2014

coveralls commented Dec 4, 2014

jwngr Dec 5, 2014

jwngr commented Dec 5, 2014

coveralls commented Dec 5, 2014

coveralls commented Dec 5, 2014

jwngr commented Dec 5, 2014

jamestalmage commented Dec 5, 2014

coveralls commented Dec 5, 2014

jwngr commented Dec 5, 2014

jamestalmage commented Dec 5, 2014

Preliminary tests for $firebaseAuth #470

Preliminary tests for $firebaseAuth #470

Conversation

jamestalmage commented Nov 18, 2014

coveralls commented Nov 18, 2014

jamestalmage commented Nov 18, 2014

jwngr commented Nov 20, 2014

jwngr commented Nov 26, 2014

coveralls commented Nov 28, 2014

coveralls commented Nov 28, 2014

coveralls commented Nov 29, 2014

coveralls commented Nov 29, 2014

coveralls commented Nov 29, 2014

jamestalmage commented Nov 29, 2014

katowulf Dec 1, 2014

Choose a reason for hiding this comment

bendrucker Dec 1, 2014

Choose a reason for hiding this comment

katowulf Dec 1, 2014

Choose a reason for hiding this comment

katowulf commented Dec 1, 2014

jamestalmage commented Dec 1, 2014

katowulf commented Dec 4, 2014

jamestalmage commented Dec 4, 2014

coveralls commented Dec 4, 2014

coveralls commented Dec 4, 2014

jwngr Dec 5, 2014

Choose a reason for hiding this comment

jwngr commented Dec 5, 2014

coveralls commented Dec 5, 2014

coveralls commented Dec 5, 2014

jwngr commented Dec 5, 2014

jamestalmage commented Dec 5, 2014

coveralls commented Dec 5, 2014

jwngr commented Dec 5, 2014

jamestalmage commented Dec 5, 2014