Closed Bug 1459161 Opened 3 years ago Closed 3 years ago

test_accounts.js doesn't correctly test for a bad call for fxa.getAssertion("noaudience")

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: standard8, Assigned: markh)

References

Details

Attachments

(1 file)

In test_accounts.js, test_getAssertion there is currently this check:

```
  do_check_throws(async function() {
    await fxa.getAssertion("nonaudience");
  });
```

do_check_throws is defined later in the file: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/services/fxaccounts/tests/xpcshell/test_accounts.js#1513-1529

There's two issues with this:

- do_check_throws isn't designed to deal with async functions, so it is unlikely that any actual throw would be caught.
- do_check_throws doesn't complain if the second parameter (result) is not passed and the function doesn't throw.

The code was added in bug 909967.

As far as I can tell, fxa.getAssertion never rejects or asserts called in this test at this point.

Looking at getAssertion's code, it appears there's various cases where it returns null, these look to have been added in bug 967120, after bug 909967.

I think the test probably has the right intentions, but is wrongly set up for this case.

Ideally, a new implementation should drop do_check_throws and use either Assert.rejects or Assert.throws instead.
No longer blocks: 1458235
Thanks Mark - I don't think that specific check has any value given the rest of the test, so I'll put a change up that removes that check and do_check_throws entirely.

I also noticed the test is doing a number of other dumb things, including pulling on Sync causing it to spew many errors, and also leaving a number of email polls still running, so I'll push a revision that fixes all of these and ask Ed to review.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Comment on attachment 8979164 [details]
Bug 1459161 - fix various small issues with FxA's test_accounts.js.

https://reviewboard.mozilla.org/r/245418/#review251420
Attachment #8979164 - Flags: review?(eoger) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/3fd5dd6c1bb4
fix various small issues with FxA's test_accounts.js. r=eoger
Blocks: 1452706
https://hg.mozilla.org/mozilla-central/rev/3fd5dd6c1bb4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.