Closed Bug 969892 Opened 8 years ago Closed 8 years ago

Some fixes to the test suite in bug 943521

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: jedp, Assigned: jedp)

References

Details

Attachments

(1 file, 9 obsolete files)

24.93 KB, patch
Details | Diff | Splinter Review
No description provided.
This patch addresses various problems that cropped up in Bug 943521.

ferjm, Can I ask for your review of this change in particular:

- FxAccounts.jsm getKeys() was returning a null promise in cases like the one tested in test_accounts.js test_overlapping_signin().  This has been fixed in FxAccounts.jsm.

ttaubert, Can I ask for your approval of these changes to test_client.js:

- Expected exceptions were not being proved before; they are now
- Removed a few straggling 'run_next_test()' calls from within add_task functions

Thank you both!
Attachment #8372871 - Flags: review?(ttaubert)
Attachment #8372871 - Flags: review?(ferjmoreno)
Comment on attachment 8372871 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Note - apply over patch for Bug 943521
Pushed to try, which seems to have shoved some other patches in there as well (not mine).  Not sure what that's about.  Hope it's ok.

https://tbpl.mozilla.org/?tree=Try&rev=21609cca9130
Comment on attachment 8372871 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Review of attachment 8372871 [details] [diff] [review]:
-----------------------------------------------------------------

Do you think we should still split the tests? I feel like it's a little more readable if we don't.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +25,5 @@
> +              "3031323334353637 38393a3b3c3d3e3f"),
> + 
> +  wrapKB:   h("4041424344454647 48494a4b4c4d4e4f"+
> +              "5051525354555657 58595a5b5c5d5e5f"),
> +};

Nit: There are some trailing/leading white spaces in here.

@@ +208,1 @@
>    }

If you expect something to fail I think it would be better to do:

try {
  lets_fail_here();
  do_throw("we shouldn't get here");
} catch (e) {
  do_check_eq(e, error)'
}

@@ +457,5 @@
> +      response.bodyOutputStream.write(existsMessage, existsMessage.length);
> +    },
> +  });
> + 
> +   let client = new FxAccountsClient(server.baseURI);

Nit: wrong indentation and a few leading white spaces around here.
Attachment #8372871 - Flags: review?(ttaubert)
Status: NEW → ASSIGNED
(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 8372871 [details] [diff] [review]
> Misc. FxAccounts test fixes, with one FxAccounts.jsm fix
> 
> Review of attachment 8372871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you think we should still split the tests? I feel like it's a little more
> readable if we don't.

I wasn't sure.  Since you think so, I've put it back how it was.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +25,5 @@
> > +              "3031323334353637 38393a3b3c3d3e3f"),
> > + 
> > +  wrapKB:   h("4041424344454647 48494a4b4c4d4e4f"+
> > +              "5051525354555657 58595a5b5c5d5e5f"),
> > +};
> 
> Nit: There are some trailing/leading white spaces in here.

Sorry about that.  (Moot now, as this part has been restored to its previous location.)

> @@ +208,1 @@
> >    }
> 
> If you expect something to fail I think it would be better to do:
> 
> try {
>   lets_fail_here();
>   do_throw("we shouldn't get here");
> } catch (e) {
>   do_check_eq(e, error)'
> }

Oh of course.  That makes so much more sense.  Thanks.

> @@ +457,5 @@
> > +      response.bodyOutputStream.write(existsMessage, existsMessage.length);
> > +    },
> > +  });
> > + 
> > +   let client = new FxAccountsClient(server.baseURI);
> 
> Nit: wrong indentation and a few leading white spaces around here.

Sorry again.  (Again, moot; restored to its former state.)

Thanks, Tim!
Attached patch 969892-misc-fixes.patch (obsolete) — Splinter Review
Addressed issues in comment 4
Attachment #8372871 - Attachment is obsolete: true
Attachment #8372871 - Flags: review?(ferjmoreno)
Attachment #8372887 - Flags: review?(ttaubert)
Attachment #8372887 - Flags: review?(ferjmoreno)
Comment on attachment 8372887 [details] [diff] [review]
969892-misc-fixes.patch

oops
Attachment #8372887 - Flags: review?(ttaubert)
Attachment #8372887 - Flags: review?(ferjmoreno)
major patch merge snafu.  sorry about that.
Tim, I've put the test vectors back at the top of the file as a const after all.
Attachment #8372887 - Attachment is obsolete: true
Attachment #8372897 - Flags: review?(ttaubert)
Attachment #8372897 - Flags: review?(ferjmoreno)
Comment on attachment 8372897 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

LGTM
Attachment #8372897 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8372897 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Review of attachment 8372897 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccounts.jsm
@@ +333,1 @@
>        if (!this.whenKeysReadyPromise) {

You can remove this if
r=ferjm
Attachment #8372897 - Attachment is obsolete: true
Attachment #8372897 - Flags: review?(ttaubert)
Attachment #8373447 - Flags: review?(ttaubert)
Comment on attachment 8373447 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Review of attachment 8373447 [details] [diff] [review]:
-----------------------------------------------------------------

The test changes look a lot better, thanks!

::: services/fxaccounts/FxAccounts.jsm
@@ +332,1 @@
>        }

For later/Off topic: we should probably rename this field to "whenKeysReadyDeferred" or something. Because a promise doesn't have a .promise property. We shouldn't do that here though :)

@@ +332,3 @@
>        }
> +      this.whenKeysReadyPromise = Promise.defer();
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {

I don't understand, how could getKeys() previously return null here? Did you check that we don't break any expectations here by returning |this.fetchAndUnwrapKeys()| that might be resolved even when abortExistingFlow() is called?

@@ +333,5 @@
> +      this.whenKeysReadyPromise = Promise.defer();
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyPromise) {
> +          this.whenKeysReadyPromise.resolve(data);
> +        }

If we can't reject fetchAndUnwrapKeys() we should probably save a local reference to the deferred created on line 333 and compare that to this.whenKeysReadyPromise before resolving. It might be a key request for an old user that makes us resolve a newer promise here, no? I know this isn't caused by your patch but we should file a follow-up for this.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +291,2 @@
>          response.setStatusLine(request.httpVersion, 200, "OK");
>          return response.bodyOutputStream.write(emptyMessage, emptyMessage.length);

Nit: put the return on a separate line so that it doesn't seem like we return for anything else than control flow reasons

@@ +293,5 @@
>        }
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
>        return response.bodyOutputStream.write(errorMessage, errorMessage.length);

Nit: remove the return here as well, please

@@ -412,5 @@
>  
>    yield deferredStop(server);
>  });
>  
> -add_task(function test_accountExists() {

Did you really want to remove the test_accountExists()? Is that covered elsewhere now and I missed it?
Attachment #8373447 - Flags: review?(ttaubert) → feedback+
(In reply to Tim Taubert [:ttaubert] from comment #14)
> Comment on attachment 8373447 [details] [diff] [review]
> Misc. FxAccounts test fixes, with one FxAccounts.jsm fix
> 
> Review of attachment 8373447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test changes look a lot better, thanks!

Thanks, Tim!

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +332,1 @@
> >        }
> 
> For later/Off topic: we should probably rename this field to
> "whenKeysReadyDeferred" or something. Because a promise doesn't have a
> .promise property. We shouldn't do that here though :)

Well, it's easy enough to fix the names!  Done for whenKeysVerifiedDeferred and whenKeysReadyDeferred.

> @@ +332,3 @@
> >        }
> > +      this.whenKeysReadyPromise = Promise.defer();
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> 
> I don't understand, how could getKeys() previously return null here? Did you
> check that we don't break any expectations here by returning
> |this.fetchAndUnwrapKeys()| that might be resolved even when
> abortExistingFlow() is called?

I think the problem was that there was no 'return' statement inside the 'if (!this.whenKeysReadyPromise)', so it was falling through to 'return this.whenKeysReadyPromise;', which was still null at that point.

I believe this is still doing the right thing for the success case: we want to return the fetchAndUnwrapKeys deferred so that down the line callers of getKeys can actually get kA etc.  (For example, test_keyFetch checks this.)

I believe if there were a problem, 'test_overlapping_signins' in test_accounts.js would catch it.  Finally, we've been doing manual testing (thank you ferjm and spenrose), and haven't encountered any problems.

> @@ +333,5 @@
> > +      this.whenKeysReadyPromise = Promise.defer();
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyPromise) {
> > +          this.whenKeysReadyPromise.resolve(data);
> > +        }
> 
> If we can't reject fetchAndUnwrapKeys() we should probably save a local
> reference to the deferred created on line 333 and compare that to
> this.whenKeysReadyPromise before resolving. It might be a key request for an
> old user that makes us resolve a newer promise here, no? I know this isn't
> caused by your patch but we should file a follow-up for this.

That's a really good point.

I've filed Bug 971173

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +291,2 @@
> >          response.setStatusLine(request.httpVersion, 200, "OK");
> >          return response.bodyOutputStream.write(emptyMessage, emptyMessage.length);
> 
> Nit: put the return on a separate line so that it doesn't seem like we
> return for anything else than control flow reasons

There were a lot of these.  All fixed.

> @@ +293,5 @@
> >        }
> >  
> >        // Second call gets an error trying to query a nonexistent account
> >        response.setStatusLine(request.httpVersion, 400, "Bad request");
> >        return response.bodyOutputStream.write(errorMessage, errorMessage.length);
> 
> Nit: remove the return here as well, please

Ditto

> @@ -412,5 @@
> >  
> >    yield deferredStop(server);
> >  });
> >  
> > -add_task(function test_accountExists() {
> 
> Did you really want to remove the test_accountExists()? Is that covered
> elsewhere now and I missed it?

Good heavens, where did it go?  It must have been obliterated when I rebased my patch somehow.  Crikey, thanks for catching that.  Reincorporated.

I've also updated the _deriveHawkCredentials test.  The original test derived the correct results, but it was not using published test vectors.  I've added the vectors to the head of the file.

Thanks for your review, Tim!
Blocks: 941723
Addresses issues in Comment 14
Attachment #8373447 - Attachment is obsolete: true
Attachment #8374334 - Flags: review?(ttaubert)
Component: Sync → FxA
Product: Firefox → Core
Blocks: 955951
No longer blocks: 941723
Comment on attachment 8374334 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Review of attachment 8374334 [details] [diff] [review]:
-----------------------------------------------------------------

I tried applying the patch to test things locally but the patch didn't apply at all. Can you please rebase, Jed?

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

Do these not throw anymore? I thought the promises would still be reject?
Attachment #8374334 - Flags: review?(ttaubert)
Thanks for looking, Tim.

They actually didn't throw to begin with.  That was a huge error with the tests.  The accountExists() method normally yields true or false (it catches the 400 response internally and checks the errno code).

Sorry the patch didn't apply.  Rebasing now.
Looks like Bug 972093 beat us to the removal of all the run_next_test() calls.

Rebased.
Attachment #8374334 - Attachment is obsolete: true
Attachment #8377669 - Flags: review?(ttaubert)
Attached patch 969892-misc-fixes.patch (obsolete) — Splinter Review
rebased
Attachment #8377669 - Attachment is obsolete: true
Attachment #8377669 - Flags: review?(ttaubert)
Attachment #8379830 - Flags: review?(ttaubert)
Comment on attachment 8379830 [details] [diff] [review]
969892-misc-fixes.patch

Review of attachment 8379830 [details] [diff] [review]:
-----------------------------------------------------------------

The tests look good to me but I'd like to get some feedback about the FxAccounts.jsm change again...

::: services/fxaccounts/FxAccounts.jsm
@@ +344,5 @@
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyDeferred) {
> +          this.whenKeysReadyDeferred.resolve(data);
> +        }
> +      });

Hmmm... I'm still not really convinced that it's the right thing to do to change that here and return .fetchAndUnwrapKeys().

.whenKeysReadyDeferred can be rejected by abortExistingFlow(). BrowserIDManager.initializeWithCurrentIdentity() can thus be aborted.

The promise returned by .fetchAndUnwrapKeys() however will not be aborted by abortExistingFlow() but will just "return null" when it encounters an unexpected generation. That would lead to initializeWithCurrentIdentity() completing the setup, no?

I don't really know the affected code too well but I don't see how the previous version could return null. Line 341 creates a deferred if there is none that then is returned on line 348. fetchAndUnwrapKeys() doesn't null it so I don't see how this function could return null in that case?

FxA.getKeys() does always return a promise, if whenKeysReadyDeferred would be null it would just resolve to "null" instead of deferring the resolution to another promise, no? Can you reproduce the error you mentioned with the old version? What is the exact error you get?

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +152,5 @@
>  
>        // Error trying to create same account a second time
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: The return here seems unneeded. Also that maybe just do:

if (created) {
  ...
} else {
  ...
}

@@ +192,5 @@
>  
>        // Error trying to sign in to nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: Same about the return here, and also if (jsonBody.email == "mé@example.com") {} else {}.

@@ +229,5 @@
>  
>        // Error trying to sign out of nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: remove return and if () {} else {}

@@ +267,5 @@
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: same :)

@@ +304,5 @@
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: same.

@@ +427,5 @@
>  
>        // Second attempt, trigger error
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: *cough*

@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

Nit: do_check_true() would be better here.
Attachment #8379830 - Flags: review?(ttaubert)
Rereading Jed's patch carefully, as best I can tell he has not changed the logic in getKeys() at all, just variable names and if block ordering. Can we review the changes he has made here, and if necessary open a separate issue to discuss how getKeys() should be structured?

(In reply to Tim Taubert [:ttaubert] from comment #21)
> Comment on attachment 8379830 [details] [diff] [review]
> 969892-misc-fixes.patch
> 
> Review of attachment 8379830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests look good to me but I'd like to get some feedback about the
> FxAccounts.jsm change again...
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +344,5 @@
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyDeferred) {
> > +          this.whenKeysReadyDeferred.resolve(data);
> > +        }
> > +      });
> 
> Hmmm... I'm still not really convinced that it's the right thing to do to
> change that here and return .fetchAndUnwrapKeys().
(In reply to Tim Taubert [:ttaubert] from comment #21)
> Comment on attachment 8379830 [details] [diff] [review]
> 969892-misc-fixes.patch

Thanks for your review, Tim!

> Review of attachment 8379830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests look good to me but I'd like to get some feedback about the
> FxAccounts.jsm change again...

Since Bug 972070 landed, the landscape has changed and I no longer feel any modification to this part of the code is necessary.  But for the record, let me respond with what I was thinking at the time.

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +344,5 @@
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyDeferred) {
> > +          this.whenKeysReadyDeferred.resolve(data);
> > +        }
> > +      });
> 
> Hmmm... I'm still not really convinced that it's the right thing to do to
> change that here and return .fetchAndUnwrapKeys().
> 
> .whenKeysReadyDeferred can be rejected by abortExistingFlow().
> BrowserIDManager.initializeWithCurrentIdentity() can thus be aborted.
> 
> The promise returned by .fetchAndUnwrapKeys() however will not be aborted by
> abortExistingFlow() but will just "return null" when it encounters an
> unexpected generation. That would lead to initializeWithCurrentIdentity()
> completing the setup, no?
> 
> I don't really know the affected code too well but I don't see how the
> previous version could return null. Line 341 creates a deferred if there is
> none that then is returned on line 348. fetchAndUnwrapKeys() doesn't null it
> so I don't see how this function could return null in that case?
> 
> FxA.getKeys() does always return a promise, if whenKeysReadyDeferred would
> be null it would just resolve to "null" instead of deferring the resolution
> to another promise, no? Can you reproduce the error you mentioned with the
> old version? What is the exact error you get?

I think the source of the problem I was wrestling with is that the promise could become nulled halfway through the spawned task in fetchAndUnwrapKeys, with the result that after falling through a test for whether the deferred did not exist, by the time the next line of code was executed, in the expectation that it existed, it suddenly did *not* exist!  This would cause test_getKeys to explode.

But all of this is moot now as markh has fixed the problem for me!  (Thanks, Mark :)

My latest patch will not have any changes to this part.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +152,5 @@
> >  
> >        // Error trying to create same account a second time
> >        response.setStatusLine(request.httpVersion, 400, "Bad request");
> > +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> > +      return;
> 
> Nit: The return here seems unneeded. Also that maybe just do:
> 
> if (created) {
>   ...
> } else {
>   ...
> }

For this and all the others you cite, the return is purely for flow control.

The useless return at the end of the block saves me from strict warnings that "anonymous function does not always return a value."  Given the choice, I personally prefer the final return to an if ... else ... construct, because it ensures that no extra code will inadvertently be tagged onto the end someday. 

But if you still prefer that I replace the 'return' statements, I'm happy to do so.  I'm not wedded to them :)

> @@ +489,5 @@
> >    let client = new FxAccountsClient(server.baseURI);
> >    let result;
> >  
> > +  result = yield client.accountExists("i.exist@example.com");
> > +  do_check_eq(result, true);
> 
> Nit: do_check_true() would be better here.

Righto.  Thanks.

Thanks again for all your time on this.  j
Attached patch Misc. FxAccounts test fixes (obsolete) — Splinter Review
Rebased
Attachment #8379830 - Attachment is obsolete: true
Attachment #8385537 - Flags: review?(ttaubert)
Comment on attachment 8385537 [details] [diff] [review]
Misc. FxAccounts test fixes

Review of attachment 8385537 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccounts.jsm
@@ +334,5 @@
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyDeferred) {
> +          this.whenKeysReadyDeferred.resolve(data);
> +        }
> +      });

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #23)
> My latest patch will not have any changes to this part.

:O

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

do_check_true()

@@ +494,3 @@
>  
> +  result = yield client.accountExists("i.also.exist@example.com");
> +  do_check_eq(result, true);

do_check_true()

@@ +497,3 @@
>  
> +  result = yield client.accountExists("i.dont.exist@example.com");
> +  do_check_eq(result, false);

do_check_false()
Attachment #8385537 - Flags: review?(ttaubert)
Thanks for taking a look, Tim.

You canceled the review - are there other problems?  Or could this be r=ttaubert if I make those three changes to test_client.js?

Thanks
j
Flags: needinfo?(ttaubert)
Please see the part where you said you will not include certain changes anymore :) Basically, r=me with those changes reverted.
Flags: needinfo?(ttaubert)
Aha!  I think I messed up in my patch export!  Thank you for your review, Tim.  Fixes forthcoming.
Attached patch Misc. FxAccounts test fixes (obsolete) — Splinter Review
r=ttaubert

Sorry about the mix-up, Tim.  I must up uploaded the last patch without doing 'hg export' first!  I can imagine you must have been wondering what I was thinking.

Cheers
j
Attachment #8385537 - Attachment is obsolete: true
Needs rebasing.
Keywords: checkin-needed
r=ttaubert

rebased
Attachment #8387855 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/945ddca04d49
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.