From 0bc03e00721eacb572474a6f2531c67a65160fd4 Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Fri, 20 Dec 2024 17:25:50 +0100 Subject: [PATCH 1/3] add suggestions --- chapter_20_mocking_1.asciidoc | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index 5aae48e1..f4c366bc 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -147,6 +147,7 @@ or what's sometimes called https://en.wikipedia.org/wiki/Monkey_patch[monkeypatc Let's suppose that, as a first step, we want to get to some code that invokes `send_mail` with the right subject line, from address, and to address. +// CSANAD: wouldn't "sender address" and "recipient address" sound better? That would look something like this: @@ -428,6 +429,7 @@ from django.test import TestCase ---- ==== +// CSANAD: using `self.assertTrue(mock_send_mail.called)` would be nicer. If you rerun the tests, you'll see they still pass. @@ -459,6 +461,9 @@ def send_login_email(request): [...] ---- ==== +// CSANAD: I suggest adding a bit of a text for the print: +// `print(f'DEBUG send_mail type:{type(send_mail)}')` +// it's arguably better practice and certainly easier for the eyes to find Let's run the tests again: @@ -645,6 +650,8 @@ def send_login_email(request): ******************************************************************************* TIP: This sidebar is an intermediate-level testing tip. +// CSANAD: not sure what sidebar we are talking about here. This TIP renders +// on the center for me. If it goes over your head the first time around, come back and take another look when you've finished this chapter. Consider also going through <> @@ -760,6 +767,8 @@ $ pass:quotes[*python src/manage.py test functional_tests.test_login*] [...] AssertionError: 'Use this link to log in' not found in 'body text tbc' ---- +// CSANAD: we haven't changed the views or the models, and we just had the unit +// tests passing before the TIP, so running it again is redundant here. We need to fill out the body text of the email, @@ -899,6 +908,7 @@ from accounts.models import Token self.assertIn(expected_url, body) ---- ==== +// CSANAD: will that be clear these go under `SendLoginEmailViewTest`? The first test is fairly straightforward; it checks that the token we create in the database @@ -1041,6 +1051,7 @@ Decoding this: * We return `None` if it doesn't. * If it does exist, we extract an email address, and either find an existing user with that address, or create a new one. +// CSANAD: shouldn't we use the numbered annotations instead? @@ -1187,7 +1198,12 @@ accounts.models.User.DoesNotExist: User matching query does not exist. ---- Let's fix each of those in turn: - +// CSANAD: I think it would be nice to explain what this error means and why we +// can fix it by returning None when it occurs. +// E.g. "The token with the given `uid` does not exist - this means we couldn't +// find the uuid in the database, so we should not authorize this login request. +// In the password-based world this would be the equivalent of an incorrect or +// missing password." [role="sourcecode"] .src/accounts/authentication.py (ch19l027) @@ -1218,6 +1234,9 @@ FAILED (errors=1) And we can handle the final case like this: +// CSANAD: again, I would add a bit more explanation: +// "If we can't find a user with the given email, then we will treat it as a +// sign-up request for a new user instead." [role="sourcecode"] .src/accounts/authentication.py (ch19l028) @@ -1428,6 +1447,11 @@ The mock.patch decorator:: Mocks can leave you tightly coupled to the implementation:: As we saw in <>, mocks can leave you tightly coupled to your implementation. +// CSANAD: this link reads as +// "As we saw in Mocks Can Leave You Tightly Coupled to the Implementation, +// mocks can leave you tightly coupled to your implementation" so it just +// repeats itself. I would re-word it e.g. "As we saw in (...), mocks can make +// your tests tied to implementation details too much." For that reason, you shouldn't use them unless you have a good reason. ******************************************************************************* From acffe79e19fe8718625d8e84294628cc3b2fbc44 Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Fri, 20 Dec 2024 17:28:55 +0100 Subject: [PATCH 2/3] add corrections: the form action and a test output was not in sync with the text explaining them --- chapter_20_mocking_1.asciidoc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index f4c366bc..ef83a9c0 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -559,6 +559,15 @@ to log in\nStart a new To-Do list' Submitting the email address currently has no effect, because the form isn't sending the data anywhere. +// CSANAD: It is sending the data! +// In the previous chapter at ch18l020 we set the form to +// `
` +// I suggest moving this changing of the hard-coded url higher up, maybe after +// ch19l004 along something like: +// "...and while we are at it, we can change the hard-coded url in the form to +// the named-path we just re-introduced from the spike into `src/accounts/urls.py`: +// `` + Let's wire it up in _base.html_: [role="sourcecode small-code"] @@ -1178,7 +1187,12 @@ class PasswordlessAuthenticationBackend: ==== -That gets one test passing but breaks another one: +That changes the `FAILED` test to also result in a "matching query does not +exist" `ERROR`. +// CSANAD: the first test was already passing even with the placeholder +// `authenticate`, the second was failing and the third resulted in an error. +// With this first cut version the passing test remains passing and the other +// two result in DoesNotExist errors. [subs="specialcharacters,macros"] From 93cedca917d7837a9ae4676f6782fbf5c1c23dce Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Fri, 20 Dec 2024 17:29:08 +0100 Subject: [PATCH 3/3] update django docs link --- chapter_20_mocking_1.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index ef83a9c0..416b203a 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -591,7 +591,7 @@ We'll use Django's "messages framework", which is often used to display ephemeral "success" or "warning" messages to show the results of an action. Have a look at the -https://docs.djangoproject.com/en/1.11/ref/contrib/messages/[django messages docs] +https://docs.djangoproject.com/en/5.1/ref/contrib/messages/[django messages docs] if you haven't come across it already. Testing Django messages is a bit contorted--we have to pass `follow=True`