Top-level promises catching is problematic when tests are failing

[eoger]

Top-level promises catching is problematic when tests are failing.

Let's say this assertion fails:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Line 876 in 5092779

assert.equal(r.obj.tokenVerificationId, null, 'tokenVerificationId is null')

This will throw and we'll end up here:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Line 895 in 5092779

testNotFound(err)

And on my terminal, mocha will print that the following assertion is failing:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Line 41 in 5092779

assert.equal(err.statusCode, 404, 'returns a 404')

The only way to know that the 1st assertion failed is open the chrome debugger and step in the file line by line.
I think this could be a super-valuable good-first-bug for an external contributor :)

[vladikoff]

@deeptibaghel , @eoger will mentor you for this bug! should be easy!

[deeptibaghel]

Sure @vladikoff , @eoger , happy to take this up :)

[eoger]

This is an example of problematic pattern:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Lines 438 to 451 in 5092779

	.then(function(r) {
	assert.equal(!! r.obj.mustVerify, true, 'mustVerify is true')
	assert.equal(r.obj.tokenVerificationId, null, 'tokenVerificationId is null')
	// Attempt to verify the session token again
	return client.postThen('/tokens/' + user.sessionToken.tokenVerificationId + '/verify', {
	uid: user.accountId
	})
	})
	.then(function () {
	assert(false, 'Verifying a verified token should have failed')
	}, function (err) {
	testNotFound(err)
	})

Here you can see that if anything fails "above" that line 450 catch, we'll still end up calling testNotFound(err).
What we want here is only to test client.postThen( on the line 443 is rejecting.
That means we should chain our rejecting promises like this:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Lines 280 to 288 in 5092779

	return client.headThen('/emailRecord/' + emailToHex(user.account.email))
	.then(function(r) {
	assert(false, 'Should not be here, since this account no longer exists')
	}, function(err) {
	assert.equal(err.toString(), 'NotFoundError', 'Account not found (no body due to being a HEAD request')
	assert.deepEqual(err.body, {}, 'Body contains nothing since this is a HEAD request')
	assert.deepEqual(err.statusCode, 404, 'Status Code is 404')
	})
	})

(Notice the .then right after the call to .headThen).

TLDR: the top level "scope" should only have .thens, so mocha can catch these failures. (feel free to ping me on IRC if that last sentence didn't make sense)

[vladikoff]

@deeptibaghel you need to break tests on purpose to see this issue

[deeptibaghel]

@deeptibaghel you need to break tests on purpose to see this issue

Yep got it, i experienced this while i was working on Buffer issue, it was returning lot of "Not found"s instead of actual failure message.

[deeptibaghel]

@eoger while working on the issue i found that this end point is not defined in api:

fxa-auth-db-mysql/db-server/test/backend/remote.js

Line 802 in 5092779

return client.getThen('/sessionToken/' + user.sessionTokenId + '/verified')

Also used here

fxa-auth-db-mysql/db-server/test/backend/remote.js

Line 846 in 5092779

return client.getThen('/sessionToken/' + user.sessionTokenId + '/verified')

is that a bug ?

[philbooth]

@deeptibaghel, good catch!

That endpoint got removed recently, looks like we missed those test cases when removing it. If you get rid of the + '/verified' bit at the end, it should work correctly! (hopefully)

[eoger]

@deeptibaghel 4808a1c was just merged and I expect it to conflict with your future PR quite a bit, so when you're done feel free to post your patch as-is and I'll rebase and resolve the conflicts (and keep your name on it of course!).