Closed Bug 983913 Opened 6 years ago Closed 6 years ago

Some assertions in test_browserid_identity aren't effective

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Adds a local Assert_rejects() function in anticipation of a real Assert.rejects
Assignee: nobody → mhammond
Attachment #8391616 - Flags: review?(ttaubert)
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: ;
Attachment #8391616 - Flags: review?(ttaubert) → feedback+
Attachment #8391620 - Flags: review?(ttaubert) → review+
Is there a bug on adding Assert.rejects to the harness?
Attachment #8391620 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c95bf0629658
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #5)
> Is there a bug on adding Assert.rejects to the harness?

Just opened bug 984172
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.