Add a test to check that a bad response from the openmarket API correctly raises an exception

[H-Shay]

As the title states.

[DMRobertson]

Many thanks for this, it's always good to improve test coverage! I think there's a mistake here though which we should correct.

I've tried to explain this and my thought process below. I, err, think I went over the top and wrote too much. I hope it's helpful and followable rather than intimidating! Please do let me know if anything's unclear and we can work through it.

[DMRobertson]

I'm slightly surprised this works. My understanding is that there are two forms of assertRaises:

• the function call form here, which will call first its argument and check that doing so raises
• the context manager form, which checks that evaluating an expression raises.

See https://docs.python.org/3/library/unittest.html?highlight=assertraises#unittest.TestCase.assertRaises

I think what's actually happening here is:

1. We evaluate the expression request.render(self.sydent.servlets.msisdnRequestCode)
2. Doing so calls self.sydent.servlets.msisdnRequestCode.render(request).
3. We eventually call this line which uses the patch/mock defined above. resp ends up being the FakeResponse created earlier.

sydent/sydent/sms/openmarket.py

Lines 105 to 107 in 567eb1a

	resp, response_body = await self.http_cli.post_json_maybe_get_json(
	API_BASE_URL, cast(JsonDict, send_body), {"headers": req_headers}
	)

4. Because resp.code == 400, we raise an Exception here:

sydent/sydent/sms/openmarket.py

Lines 121 to 129 in 567eb1a

	if resp.code < 200 or resp.code >= 300:
	if response_body is None or "error" not in response_body:
	raise Exception(
	"OpenMarket API responded with status %d (request ID: %s)"
	% (
	resp.code,
	request_id,
	),
	)

5. BUT, crucially, that gets caught in the second except block here:

sydent/sydent/http/servlets/msisdnservlet.py

Lines 101 to 121 in d702647

	try:
	sid = await self.sydent.validators.msisdn.requestToken(
	phone_number_object, clientSecret, sendAttempt, brand
	)
	resp = {
	"success": True,
	"sid": str(sid),
	"msisdn": msisdn,
	"intl_fmt": intl_fmt,
	}
	except DestinationRejectedException:
	logger.warning("Destination rejected for number: %s", msisdn)
	request.setResponseCode(400)
	resp = {
	"errcode": "M_DESTINATION_REJECTED",
	"error": "Phone numbers in this country are not currently supported",
	}
	except Exception:
	logger.exception("Exception sending SMS")
	request.setResponseCode(500)
	resp = {"errcode": "M_UNKNOWN", "error": "Internal Server Error"}

6. render_POST continues execution and returns None.
7. twisted.web.server.Request.render continues execution and returns None.
8. Having evaluated the second argument to assertRaises, we then call self.assertRaises(Exception, None).
9. Doing so tries to call None(). This raises TypeError: 'NoneType' object is not callable.
10. TypeError is a subclass of Exception, so assertRaises sees the behaviour it expects. (C.f. the stdlib's exception hierarchy.)
11. The test passes.

Sorry, this has become a wall of text. I think there are two key corrections here:

• assertRaises in the form used here first evaluates and then calls its second argument. To check that evaluating an expression/or statements raises an exception, use the context manager form.
• assertRaises checks that something raises an exception which is not suppressed. Any exception that are raised, caught and not reraised (points 4 and 5) are an implementation detail and not part of the externally testable behaviour.

[DMRobertson]

(As a sanity check, try making the mock return 200 instead of 400. I think you'll see that the test still passes and that we are therefore insane.)

I suggest taking a moment to consider what is under test here, and what the test wants to ensure. We agree on the scenario: the openmarket API returns 400.

• What thing are we testing in this circumstance?
• What is the behaviour we want to ensure occurs? (I.e. what can we detect/measure?)

It looks to me like we're testing Sydent's handling of the request as a whole, as if we're a user of the API. The user knows that something has gone wrong only by inspecting the HTTP response. I suggest this test should do so too; I think we can do this by interrogating the channel.

Note: your instinct is good here! It would be very reasonable to test that sendTextSMS raises an Exception when its call to post_json_maybe_get_json returns 400. It'd make for a great unit test of sydent.sms.openmarket. What we have above---making the request and checking the response---is higher level, and tests multiple different layers interacting together. It's more of an integration or functional test.

[H-Shay]

Thank you for this write-up! I really appreciate it, and it is very helpful/edifying!

[DMRobertson]

LGTM! (I hope the write-up was useful)

[DMRobertson]

Out of interest, is this AsyncMock on 3.8+ and MagicMock otherwise?

[H-Shay]

Yes that was the idea, although I think I changed it to MagicMock and mypy is still happy.

[DMRobertson]

I'd guess that patch isn't annotated so it can't really check the details. And also,AsyncMock and MagicMock are so magic anyway that, erm, I'm not sure there's much for it to check. Maybe I was a little too keen there!