983913 - Some assertions in test_browserid_identity aren't effective

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

eg:

  try {
    yield browseridManager.whenReadyToAuthenticate.promise;
    Assert.ok(false, "expecting this promise to resolve with an error");
  } catch (ex) {}

isn't doing what it appears.  Assert.ok simply throws if it fails, and the exception handler consumes the exception.

FTR, if the Assert.ok used do_check_true, things *do* work as expected due to how the do_check functions are implemented.

Blocks bug 983270 as that wants to refactor some test code.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-983913-ensure-failure-to-reject-a-promise-is-act.patch

Adds a local Assert_rejects() function in anticipation of a real Assert.rejects

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8391616 [details] [diff] [review]
0001-Bug-983913-ensure-failure-to-reject-a-promise-is-act.patch

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

Assert_rejects() shouldn't be a generator. It can just return a promise like done below. Other than that the approach looks good!

::: services/sync/tests/unit/test_browserid_identity.js
@@ +30,5 @@
> +  } catch (_) {
> +    threw = true;
> +  }
> +  Assert.ok(threw, message || "Expected the promise to be rejected");
> +}

function Assert_rejects(promise, message) {
  let deferred = Promise.defer();
  promise.then(() => deferred.reject(message || "Expected ..."), deferred.resolve);
  return deferred.promise;
}

@@ +265,5 @@
>    browseridManager.initializeWithCurrentIdentity();
>    Assert.ok(!browseridManager._shouldHaveSyncKeyBundle,
>              "_shouldHaveSyncKeyBundle should be false so we know we are testing what we think we are.");
>    Status.login = LOGIN_FAILED_NO_USERNAME;
> +  yield Assert_rejects(browseridManager.ensureLoggedIn(), "expecting rejection due to no user")

Nit: ;

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-983913-ensure-failure-to-reject-a-promise-is-act.patch

Comment 4

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/c95bf0629658

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Is there a bug on adding Assert.rejects to the harness?

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c95bf0629658

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #5)
> Is there a bug on adding Assert.rejects to the harness?

Just opened bug 984172

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bdf916495633