From e95a504f92ca21a6ed31b22d515fbe3b66f313a1 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 30 Oct 2024 10:54:31 +0000 Subject: [PATCH 01/63] Start on updates to 23, talk about testing server and docker --- chapter_23_debugging_prod.asciidoc | 224 +++++++++++++++++++++++++---- 1 file changed, 194 insertions(+), 30 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 06626a11..7baf4fd9 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1,5 +1,5 @@ [[chapter_23_debugging_prod]] -== Server-Side Debugging +== Debugging Production Issues .Warning, Chapter Update in Progress ******************************************************************************* @@ -76,28 +76,66 @@ FAILED (failures=1, errors=1) ---- -We can't log in--either with the real email system or with our -pre-authenticated session. Looks like our nice new authentication -system is crashing the server. +We can't log in--either with the real email system or with our pre-authenticated session. +Looks like our nice new authentication system is crashing the server. -Let's practice a bit of server-side debugging! +Let's practice a bit of production debugging! -* TODO: show we can actually repro this with Docker. +=== Trying to Repro in Docker + +One of the reasons we went to the trouble of building a Docker image, +was to have a way of simulating what's on the server as closely as possible, locally. +So let's see whether we get the same error if we test against Docker. + +Let's rebuild and start our Docker container locally, +on port 8888: + +[subs="specialcharacters,quotes"] +---- +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -it superlists* +---- + +And now let's see if our errors repro against Docker: + + +[role="small-code"] +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] +[...] +selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] +[...] +AssertionError: 'Check your email' not found in 'Server Error (500)' +[...] +FAILED (failures=1, errors=1) +---- + +Sure enough, same two errors! // TODO: actually, does this obviate the whole need for running fts against the server? -=== Inspecting Logs on the Server +=== Inspecting the Docker Container Logs ((("logging"))) ((("Gunicorn", "logging setup"))) -In order to track this problem down, -we need to get some logging information out of Django. +When Django fails with a 500 or "Unhandled Exception" and DEBUG is off, +it doesn't print the tracebacks to your web browser. +But it will send them to your logs instead. + +.Check our Django LOGGING settings +******************************************************************************* -First, make sure your 'settings.py' still contains the `LOGGING` -settings which will actually send stuff to the console: +It's worth double checking at this point that your _settings.py_ +still contains the `LOGGING` settings which will actually send stuff +to the console: [role="sourcecode currentcontents"] .src/superlists/settings.py @@ -117,44 +155,174 @@ LOGGING = { ---- ==== -Restart the Docker container again if necessary, +Restart the Docker container if necessary, and then either rerun the FT, or just try to log in manually. -While that happens, we watch the logs on the server with `docker logs -f`: +******************************************************************************* + +If you switch to the terminal that's running your Docker image, +you should see the traceback printed out in there: + +[role="skipme"] +[subs="specialcharacters,quotes"] +---- +Internal Server Error: /accounts/send_login_email +Traceback (most recent call last): +[...] + File "/src/accounts/views.py", line 16, in send_login_email + send_mail( + ~~~~~~~~~^ + "Your login link for Superlists", + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + ...<2 lines>... + [email], + ^^^^^^^^ + ) + ^ +[...] + self.connection.sendmail( + ~~~~~~~~~~~~~~~~~~~~~~~~^ + from_email, recipients, message.as_bytes(linesep="\r\n") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + ) + ^ + File "/usr/local/lib/python3.13/smtplib.py", line 876, in sendmail + raise SMTPSenderRefused(code, resp, from_addr) +smtplib.SMTPSenderRefused: (530, b'5.7.0 Authentication Required. [...] +---- + +That looks like a pretty good clue to what's going on. + +Before we go further, it's worth confirming that the error on the actual server +is the same as the one we see in Docker. + +SSH in to your server and run `docker logs`: [role="server-commands"] [subs="specialcharacters,quotes"] ---- -elspeth@server:$ *docker logs -f superlists* +elspeth@server:$ *docker logs superlists* ---- You should see an error like this: [role="skipme small-code"] [subs="specialcharacters,quotes"] ---- +❯ ssh elspeth@staging.ottg.co.uk docker logs superlists +[2024-10-30 09:55:08 +0000] [6] [INFO] Starting gunicorn 22.0.0 +[2024-10-30 09:55:08 +0000] [6] [INFO] Listening at: http://0.0.0.0:8888 (6) +[2024-10-30 09:55:08 +0000] [6] [INFO] Using worker: sync +[2024-10-30 09:55:08 +0000] [7] [INFO] Booting worker with pid: 7 +Not Found: /favicon.ico +Not Found: /favicon.ico +Not Found: /favicon.ico +Not Found: /favicon.ico +Not Found: /favicon.ico Internal Server Error: /accounts/send_login_email Traceback (most recent call last): - File "/home/elspeth/sites/staging.ottg.co.uk/.venv/lib/python3.7/[...] + File "/venv/lib/python3.13/site-packages/django/core/handlers/exception.py", + line 55, in inner + response = get_response(request) + File "/venv/lib/python3.13/site-packages/django/core/handlers/base.py", line + 197, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) - File -"/home/elspeth/sites/staging.ottg.co.uk/accounts/views.py", line -20, in send_login_email - [email] + File "/src/accounts/views.py", line 16, in send_login_email + send_mail( + ~~~~~~~~~^ + "Your login link for Superlists", + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] - self.connection.sendmail(from_email, recipients, message.as_bytes(linesep='\r\n')) - File "/usr/lib/python3.7/smtplib.py", line 862, in sendmail raise SMTPSenderRefused(code, resp, from_addr) -smtplib.SMTPSenderRefused: (530, b'5.5.1 Authentication Required. Learn more -at\n5.5.1 https://support.google.com/mail/?p=WantAuthError [...] -- gsmtp', 'noreply@superlists') +smtplib.SMTPSenderRefused: (530, b'5.7.0 Authentication Required. [...] ---- -Hm, Gmail is refusing to send our emails, is it? Now why might that be? -Ah yes, we haven't told the server what our password is! + +Sure enough! Good to know our local Docker setup can repro the error on the server. ((("", startref="SScatch21")))((("", startref="DBserstag21"))) === Another Environment Variable +Hm, Gmail is refusing to send our emails, is it? Now why might that be? +Ah yes, we haven't told the server what our password is! + +Let's add this new environment variable to our local Docker container `run` +command: + +First, set your email password in your terminal if you need to: + +[subs="specialcharacters,quotes"] +---- +$ *export EMAIL_PASSWORD="yoursekritpasswordhere"* +---- + +Now, rebuild and re-run our container, with an extra `-e` flag: + +[subs="specialcharacters,quotes"] +---- +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD="$EMAIL_PASSWORD" \ + -it superlists* +---- + +And now we can rerun our FT again. +We'll narrow it down to just the `test_login` test since that's the main one that has a problem: + +[role="small-code"] +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests.test_login*] +[...] +ERROR: test_login_using_magic_link +(functional_tests.test_login.LoginTest.test_login_using_magic_link) + --------------------------------------------------------------------- +Traceback (most recent call last): + File "...goat-book/src/functional_tests/test_login.py", line 32, in +test_login_using_magic_link + email = mail.outbox.pop() +IndexError: pop from empty list +---- + +Aha! The tests get a little further. +It looks like our server _can_ now send emails, +(and the docker log no longer shows any errors), +they're just not appearing in `mail.outbox`. + +The reason is that `mail.outbox` is a local, in-memory variable in Django, +so that's only going to work when our tests and our server are running in the same process, like they do with unit tests or with `LiveServerTestCase` FTs. + +When we run against another process, be it Docker or an actual server, +we can't access the same `mail.outbox` variable. + +We need another technique if we want to actually inspect the emails +that the server sends, in our tests against Docker or the server. + + +=== Deciding How to Test "Real" Email Sending + +This is a point at which we have to explore some tradeoffs. +There are a few different ways we could test this: + +1. We could build a "real" end-to-end test, and have our tests + log in to an email server, and retrieve the email from there. + That's what I did in the first and second edition. + +2. We could give up on testing email on the server. + If we have a minimal smoke test that the server _can_ send emails, + then we don't need to test that they are actually delivered. + +3. We can use an alternative, fake email backend, + whereby Django will save the emails to a file on disk for example, + and we can inspect them there. + + + + +=== Setting Secret Environment Variables on the Server + ((("debugging", "server-side", "setting secret environment variables"))) ((("environment variables"))) ((("secret values"))) @@ -203,10 +371,6 @@ but we can't check the email in the `mail.outbox`... ((("debugging", "server-side", "testing POP3 emails", id="DBservemail21"))) ((("Django framework", "sending emails", id="DJFemail21"))) ((("emails, sending from Django", id="email21"))) -Ah. That explains it. -Now that we're running against a real server rather than the `LiveServerTestCase`, -we can no longer inspect the local `django.mail.outbox` to see sent emails. - First, we'll need to know, in our FTs, whether we're running against the staging server or not. From 7ed005753a0154fad125ec62e3e085b0d3b8d1d5 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 6 Nov 2024 18:25:41 +0000 Subject: [PATCH 02/63] more in 23 --- chapter_23_debugging_prod.asciidoc | 209 +++++++++++++++++++---------- 1 file changed, 141 insertions(+), 68 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 7baf4fd9..abff3808 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -240,11 +240,26 @@ Sure enough! Good to know our local Docker setup can repro the error on the ser ((("", startref="SScatch21")))((("", startref="DBserstag21"))) -=== Another Environment Variable +=== Another Environment Variable In Docker -Hm, Gmail is refusing to send our emails, is it? Now why might that be? +So, Gmail is refusing to send our emails, is it? Now why might that be? Ah yes, we haven't told the server what our password is! + +As you might remember from earlier chapters, +our _settings.py_ expects to get the email server password from an environment variable +named `EMAIL_PASSWORD`: + +[role="sourcecode currentcontents"] +.src/superlists/settings.py +==== +[source,python] +---- +EMAIL_HOST_PASSWORD = os.environ.get("EMAIL_PASSWORD") +---- +==== + + Let's add this new environment variable to our local Docker container `run` command: @@ -252,10 +267,14 @@ First, set your email password in your terminal if you need to: [subs="specialcharacters,quotes"] ---- +$ *echo $EMAIL_PASSWORD* +# if that's empty, let's set it: $ *export EMAIL_PASSWORD="yoursekritpasswordhere"* ---- -Now, rebuild and re-run our container, with an extra `-e` flag: +Now let's pass that env var thru to our docker container using one more `-e` flag, +this one fishing the env var out of the shell we're in: + [subs="specialcharacters,quotes"] ---- @@ -264,10 +283,15 @@ $ *docker build -t superlists . && docker run \ --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ -e DJANGO_SECRET_KEY=sekrit \ -e DJANGO_ALLOWED_HOST=localhost \ - -e EMAIL_PASSWORD="$EMAIL_PASSWORD" \ + -e EMAIL_PASSWORD \ <1> -it superlists* ---- +<1> If you use `-e` without an `=something` argument, it sets the env var inside Docker + to the same value set in the current shell. + It's like saying `-e EMAIL_PASSWORD=$EMAIL_PASSWORD` + + And now we can rerun our FT again. We'll narrow it down to just the `test_login` test since that's the main one that has a problem: @@ -319,6 +343,119 @@ There are a few different ways we could test this: and we can inspect them there. +Let's explore all three options and pick one. + +=== How to Test Email End-To-End, if You Want To + +Here's an example helper function that can retrieve a real email +from a real POP3 email server, +using the horrifically tortuous Python standard library POP3 client. + +To make it work, we'll need an email address to receive the email. +I signed up for a Yahoo account for testing, +but you can use any email service you like, as long as it offers POP3 access. + +You will need to set the +`RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. + +[subs="specialcharacters,quotes"] +---- +$ *echo RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere >> .env* +$ *set -a; source .env; set +a* +---- + +[role="sourcecode skipme"] +.src/functional_tests/test_login.py (ch23l001) +==== +[source,python] +---- +import os +import poplib +import re +impot time +[...] + +def retrieve_pop3_email(receiver_email, subject, pop3_server, pop3_password): + email_id = None + start = time.time() + inbox = poplib.POP3_SSL(pop3_server) + try: + inbox.user(receiver_email) + inbox.pass_(pop3_password) + while time.time() - start < POP3_TIMEOUT: + # get 10 newest messages + count, _ = inbox.stat() + for i in reversed(range(max(1, count - 10), count + 1)): + print("getting msg", i) + _, lines, __ = inbox.retr(i) + lines = [l.decode("utf8") for l in lines] + print(lines) + if f"Subject: {subject}" in lines: + email_id = i + body = "\n".join(lines) + return body + time.sleep(5) + finally: + if email_id: + inbox.dele(email_id) + inbox.quit() +---- +==== + +If you're curious, I'd encourage you to try this out in your FTs. +It definitely _can_ work. +But, having tried it in the first couple of editions of the book. +I have to say it's fiddly to get right, +and often flaky, which is a highly undesirable property for a testing tool. + + +=== Using a Fake Email Backend For Django + +Let's say that, if we detect an environment variable `EMAIL_FILE_PATH`, +we switch to Django's file-based backend: + + +.src/superlists/settings.py +==== +[source,python] +---- +EMAIL_HOST = "smtp.gmail.com" +EMAIL_HOST_USER = "obeythetestinggoat@gmail.com" +EMAIL_HOST_PASSWORD = os.environ.get("EMAIL_PASSWORD") +EMAIL_PORT = 587 +EMAIL_USE_TLS = True +# Use fake file-based backend if EMAIL_FILE_PATH is set +if "EMAIL_FILE_PATH" in os.environ: + EMAIL_BACKEND = "django.core.mail.backends.filebased.EmailBackend" + EMAIL_FILE_PATH = os.environ["EMAIL_FILE_PATH"] +---- +==== + +Now let's set that file path, and mount it inside our docker container, +so that it's available both inside and outside the container: + +[subs="specialcharacters,quotes"] +---- +# set a local env var for our path to the emails file +$ *export EMAIL_FILE_PATH=/tmp/superlists-emails* +# make sure the file exists +$ *mkdir -p $EMAIL_FILE_PATH* +# re-run our container, with the EMAIL_FILE_PATH as an env var, and mounted. +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ <1> + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ <2> + -it superlists* +---- + +<1> Here's where we mount the emails file so we can see it + both inside and outside the container + +<2> And here's where we pass the path as an env var. === Setting Secret Environment Variables on the Server @@ -389,70 +526,6 @@ Let's save the `staging_server` variable on `self` in _base.py_: ---- ==== -Then we build a helper function that can retrieve a real email from a real POP3 -email server, using the horrifically tortuous Python standard library POP3 -client: - -[role="sourcecode"] -.src/functional_tests/test_login.py (ch21l010) -==== -[source,python] ----- -import os -import poplib -import re -import time -[...] - - def wait_for_email(self, test_email, subject): - if not self.test_server: - email = mail.outbox.pop() - self.assertIn(test_email, email.to) - self.assertEqual(email.subject, subject) - return email.body - - email_id = None - start = time.time() - inbox = poplib.POP3_SSL("pop.mail.yahoo.com") - try: - inbox.user(test_email) - inbox.pass_(os.environ["YAHOO_PASSWORD"]) - while time.time() - start < 60: - # get 10 newest messages - count, _ = inbox.stat() - for i in reversed(range(max(1, count - 10), count + 1)): - print("getting msg", i) - _, lines, __ = inbox.retr(i) - lines = [l.decode("utf8") for l in lines] - print(lines) - if f"Subject: {subject}" in lines: - email_id = i - body = "\n".join(lines) - return body - time.sleep(5) - finally: - if email_id: - inbox.dele(email_id) - inbox.quit() ----- -==== - -* TODO: consider not using POP3, maybe - https://docs.djangoproject.com/en/5.0/topics/email/#file-backend[file backend] instead. - discuss tradeoff of testing config - vs not needing to test that django can actually send emails - -I signed up for a Yahoo account for testing, -but you can use any email service you like, as long as it offers POP3 access. -You will need to set the -`YAHOO_PASSWORD` environment variable in the console that's running the FT. - -[subs="specialcharacters,quotes"] ----- -$ *echo YAHOO_PASSWORD=otheremailpasswordhere >> .env* -$ *set -a; source .env; set +a* ----- - And then we feed through the rest of the changes to the FT that are required as a result. Firstly, populating a `test_email` variable, differently for local and staging tests: From 8963d7cc457bde7ed1d3ebb16b19df4907176dd5 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 13 Nov 2024 17:28:08 +0000 Subject: [PATCH 03/63] quieten pyright --- pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index b6cc8b45..037a6a0c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -55,3 +55,5 @@ ignore = [ # these help pyright in neovide to find its way around venvPath = "." venv = ".venv" +# most of the source for the book itself is untyped +typeCheckingMode = "standard" From fe3f6e764b2c09ee0e687fce807216cb9209cf0f Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 13 Nov 2024 17:28:42 +0000 Subject: [PATCH 04/63] mention mailinatory et al --- chapter_23_debugging_prod.asciidoc | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index abff3808..b5a801c6 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -334,18 +334,28 @@ There are a few different ways we could test this: log in to an email server, and retrieve the email from there. That's what I did in the first and second edition. +2. You can use a service like Mailinator or Mailsac, + which give you an email account to send to, + and some APIs for checking what mail has been delivered. + 2. We could give up on testing email on the server. If we have a minimal smoke test that the server _can_ send emails, - then we don't need to test that they are actually delivered. + then we don't need to test that they are _actually_ delivered. 3. We can use an alternative, fake email backend, whereby Django will save the emails to a file on disk for example, and we can inspect them there. -Let's explore all three options and pick one. +I'm not going to explore option 2 in this book, +since it involves a commercial service and I don't want to endorse one, +but that's not to say it's a bad option. +Especially since they have free plans these days! + +But let's explore the other three options (1, 2 and 4) and their pros+cons. -=== How to Test Email End-To-End, if You Want To + +=== How to Test Email End-To-End with POP3 Here's an example helper function that can retrieve a real email from a real POP3 email server, @@ -407,6 +417,11 @@ It definitely _can_ work. But, having tried it in the first couple of editions of the book. I have to say it's fiddly to get right, and often flaky, which is a highly undesirable property for a testing tool. +So let's leave that there for now. + +TIP: If you _do_ want to test email end-to-end, + I'd encourage you to investigate services like Mailinator or Mailsac, + rather than trying to use POP3 directly. === Using a Fake Email Backend For Django From 392c6bf8326092eff931ecff3d867ad7cc7ac0ca Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 13 Nov 2024 17:28:57 +0000 Subject: [PATCH 05/63] work thru how to test with file --- chapter_23_debugging_prod.asciidoc | 133 ++++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 2 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index b5a801c6..35530945 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -430,7 +430,7 @@ Let's say that, if we detect an environment variable `EMAIL_FILE_PATH`, we switch to Django's file-based backend: -.src/superlists/settings.py +.src/superlists/settings.py (ch23l002) ==== [source,python] ---- @@ -446,6 +446,105 @@ if "EMAIL_FILE_PATH" in os.environ: ---- ==== +Here's how we can adapt our tests to conditionally use the email file, +instead of Django's `mail.outbox`, if the env var is set when running our tests: + + + +[role="sourcecode"] +.src/functional_tests/test_login.py (ch23l003) +==== +[source,python] +---- +class LoginTest(FunctionalTest): + def retrieve_email_from_file(self, sent_to, subject, emails_dir): # <1> + latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] # <2> + latest_email = latest_emails_file.read_text().split("-" * 80)[-1] # <3> + self.assertIn(subject, latest_email) + self.assertIn(sent_to, latest_email) + return latest_email + + def retrieve_email_from_django_outbox(self, sent_to, subject): # <4> + email = mail.outbox.pop() + self.assertIn(sent_to, email.to) + self.assertEqual(email.subject, subject) + return email.body + + def wait_for_email(self, sent_to, subject): # <5> + """ + Retrieve email body, + from a file if the right env var is set, + or get it from django.mail.outbox by default + """ + if email_file_path := os.environ.get("EMAIL_FILE_PATH"): # <6> + return self.wait_for( # <7> + lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) + ) + else: + return self.retrieve_email_from_django_outbox(sent_to, subject) + + def test_login_using_magic_link(self): + [...] +---- +==== + +<1> Here's our helper method for getting email contents from a file. + It takes the configured email directory as an argument, + as well as the sent-to address and expected subject. + +<2> Django saves a new file with emails every time you restart the server. + The filename has a timestamp in it, + so we can get the latest one by sorting the files in our test directory. + Check out the https://docs.python.org/3/library/pathlib.html[Pathlib] docs + if you haven't used it before, it's a nice, relatively new way of working with files in Python. + +<3> The emails in the file are separated by a line of 80 hyphens. + +<4> This is the matching helper for getting the email from `mail.outbox`. + +<5> Here's where we dispatch to the right helper based on whether the env + var is set. + +<6> Checking whether an environment variable is set, and using its value if so, + is one of the (relatively few) places where it's nice to use the walrus operator. + +<7> I'm using a `wait_for()` here because anything involving reading and writing from files, + especially across the filesystem mounts inside and outside of Docker, + has a potential race condition. + + +We'll need a couple more minor changes to the FT, to use the helper: + + +[role="sourcecode"] +.src/functional_tests/test_login.py (ch23l004) +==== +[source,diff] +---- +@@ -59,15 +59,12 @@ class LoginTest(FunctionalTest): + ) + + # She checks her email and finds a message +- email = mail.outbox.pop() +- self.assertIn(TEST_EMAIL, email.to) +- self.assertEqual(email.subject, SUBJECT) ++ email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) + + # It has a URL link in it +- self.assertIn("Use this link to log in", email.body) +- url_search = re.search(r"http://.+/.+$", email.body) +- if not url_search: +- self.fail(f"Could not find url in email body:\n{email.body}") ++ self.assertIn("Use this link to log in", email_body) ++ if not (url_search := re.search(r"http://.+/.+$", email_body, re.MULTILINE)): ++ self.fail(f"Could not find url in email body:\n{email_body}") + url = url_search.group(0) + self.assertIn(self.live_server_url, url) +---- +==== + +// TODO backport that walrus + Now let's set that file path, and mount it inside our docker container, so that it's available both inside and outside the container: @@ -470,7 +569,37 @@ $ *docker build -t superlists . && docker run \ <1> Here's where we mount the emails file so we can see it both inside and outside the container -<2> And here's where we pass the path as an env var. +<2> And here's where we pass the path as an env var, + once again re-exporting the variable from the current shell. + + +And we can re-run our FT, first without using Docker or the EMAIL_FILE_PATH, +just to check we didn't break anything: + + +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*./src/manage.py test functional_tests.test_login*] +[...] +OK +---- + +And now _with_ Docker and the EMAIL_FILE_PATH. Remember, + +[subs="specialcharacters,quotes"] +---- +# we need to set the EMAIL_FILE_PATH in this terminal too +$ *export EMAIL_FILE_PATH=/tmp/superlists-emails* +$ *TEST_SERVER=localhost:8888 python src/manage.py test functional_tests* +[...] +OK +---- + + +It works! Hooray. + + +* TODO resume here === Setting Secret Environment Variables on the Server From 1996aa29057521b7743e9bf86bd350ab4af36b97 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 13 Nov 2024 18:09:44 +0000 Subject: [PATCH 06/63] show deliberately breaking test --- chapter_23_debugging_prod.asciidoc | 157 ++++++++++++++++++++++++++++- 1 file changed, 156 insertions(+), 1 deletion(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 35530945..cab50ee2 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -599,7 +599,162 @@ OK It works! Hooray. -* TODO resume here +==== Double-Checking our Test and Our Fix + +As always, we should be suspicious of any test that we've only ever seen pass! +Let's see if we can make this test fail. + +NOTE: You might have lost track of the actual bug and how we fixed it! + The bug was, the server was crashing when it tried to send an email. + The reason was, we hadn't set the `EMAIL_PASSWORD` environment variable. + So the actual fix is to set that env var, + and the way we _test_ that it works, is by using the `filebased.EmailBackend" + `EMAIL_BACKEND` setting using the `EMAIL_FILE_PATH` environment variable. + + +So, how shall we make the test fail? +Well, how about if we deliberately break the email that the server sends: + +TODO: filename/commit + +[role="sourcecode"] +.lists.tests.py (ch04l004) +==== +[source,python] +---- +def send_login_email(request): + email = request.POST["email"] + token = Token.objects.create(email=email) + url = request.build_absolute_uri( + reverse("login") + "?token=" + str(token.uid), + ) + message_body = f"Use this link to log in:\n\n{url}" + send_mail( + "Your login link for Superlists", + "HAHA NO LOGIN URL FOR U", # <1> + "noreply@superlists", + [email], + ) + messages.success( + request, + "Check your email, we've sent you a link you can use to log in.", + ) + return redirect("/") +---- +==== + +<1> This should do it! We'll still send an email, + but it won't contain a login URL. + + +* TODO: aside on moujnting /src/? + +So let's try it: + + +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails ./src/manage.py test functional_tests.test_login +[...] +Ran 1 test in 2.513s + +OK +---- + +==== Testing side-effects is fiddly! + +TODO: flesh out explanation + +eh? what's happening? + +It's because we're picking up an old email, which is still a valid token in the DB + + +Let's clear out the db: + +[subs="specialcharacters,quotes"] +---- +$ *rm src/db.sqlite3 && ./src/manage.py migrate* +Operations to perform: + Apply all migrations: accounts, auth, contenttypes, lists, sessions +Running migrations: + Applying accounts.0001_initial... OK + Applying accounts.0002_token... OK + Applying contenttypes.0001_initial... OK + Applying contenttypes.0002_remove_content_type_name... OK + Applying auth.0001_initial... OK +---- + + +And... + +cmdgg +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* +[...] +ERROR: test_login_using_magic_link (functional_tests.test_login.LoginTest.test_login_using_magic_link) + self.wait_to_be_logged_in(email=TEST_EMAIL) + ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^ +[...] +selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] +---- + +OK that's weird, it _does_ still find an email with a magic link in? + +ah, it's an old one. + + +//// +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* +ERROR: test_login_using_magic_link +(functional_tests.test_login.LoginTest.test_login_using_magic_link) +[...] + email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) +[...] + return self.wait_for( + ~~~~~~~~~~~~~^ + lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +[...] + latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^ +IndexError: list index out of range +---- + +That's better! We're not sending any emails, so there's no email file to find. +//// + +Let's delete all our old emails + +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*rm $EMAIL_FILE_PATH/*] +---- + +And now re rerun the FT: + +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] +FAIL: test_login_using_magic_link +(functional_tests.test_login.LoginTest.test_login_using_magic_link) +[...] + email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) +[...] + self.assertIn("Use this link to log in", email_body) + ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +AssertionError: 'Use this link to log in' not found in 'Content-Type: +text/plain; charset="utf-8"\nMIME-Version: 1.0\nContent-Transfer-Encoding: +7bit\nSubject: Your login link for Superlists\nFrom: noreply@superlists\nTo: +edith@example.com\nDate: Wed, 13 Nov 2024 18:00:55 -0000\nMessage-ID: +[...]\n\nHAHA NO LOGIN URL FOR +U\n-------------------------------------------------------------------------------\n' +---- + + +That's the error we wanted! === Setting Secret Environment Variables on the Server From 129359810841aee5c98b3d4e5a96c40a61e291bd Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 20 Nov 2024 18:01:10 +0000 Subject: [PATCH 07/63] More in 23. getting to be a pain --- chapter_23_debugging_prod.asciidoc | 395 +++++++++++++---------------- 1 file changed, 182 insertions(+), 213 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index cab50ee2..5c578b68 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -584,13 +584,12 @@ $ pass:quotes[*./src/manage.py test functional_tests.test_login*] OK ---- -And now _with_ Docker and the EMAIL_FILE_PATH. Remember, +And now _with_ Docker and the EMAIL_FILE_PATH: [subs="specialcharacters,quotes"] ---- -# we need to set the EMAIL_FILE_PATH in this terminal too -$ *export EMAIL_FILE_PATH=/tmp/superlists-emails* -$ *TEST_SERVER=localhost:8888 python src/manage.py test functional_tests* +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ + python src/manage.py test functional_tests* [...] OK ---- @@ -599,26 +598,36 @@ OK It works! Hooray. -==== Double-Checking our Test and Our Fix +=== Double-Checking our Test and Our Fix As always, we should be suspicious of any test that we've only ever seen pass! Let's see if we can make this test fail. -NOTE: You might have lost track of the actual bug and how we fixed it! - The bug was, the server was crashing when it tried to send an email. - The reason was, we hadn't set the `EMAIL_PASSWORD` environment variable. - So the actual fix is to set that env var, - and the way we _test_ that it works, is by using the `filebased.EmailBackend" - `EMAIL_BACKEND` setting using the `EMAIL_FILE_PATH` environment variable. - - -So, how shall we make the test fail? -Well, how about if we deliberately break the email that the server sends: +Before we do--we've been in the detail for a bit, +it's worth reminding ourselves of what the actual bug was, +and how we're fixing it! +The bug was, the server was crashing when it tried to send an email. +The reason was, we hadn't set the `EMAIL_PASSWORD` environment variable. +We managed to repro the bug in Docker. +The actual _fix_ is to set that env var, +both in Docker and eventually on the server. +Now we want to have a _test_ that our fix works, +and we looked in to a few different options, +settling on using the `filebased.EmailBackend" +`EMAIL_BACKEND` setting using the `EMAIL_FILE_PATH` environment variable. + +Now, I say we haven't seen the test fail, +but actually we have, when we repro'd the bug. +If we unset the `EMAIL_PASSWORD` env var, it will fail again. +I'm more worried about the new parts of our tests, +the bits where we go and read from the file at `EMAIL_FILE_PATH`. +How can we make that part fail? + +Well, how about if we deliberately break our email-sending code? -TODO: filename/commit [role="sourcecode"] -.lists.tests.py (ch04l004) +.src/accounts/views.py (ch23l005) ==== [source,python] ---- @@ -629,12 +638,12 @@ def send_login_email(request): reverse("login") + "?token=" + str(token.uid), ) message_body = f"Use this link to log in:\n\n{url}" - send_mail( - "Your login link for Superlists", - "HAHA NO LOGIN URL FOR U", # <1> - "noreply@superlists", - [email], - ) + # send_mail( <1> + # "Your login link for Superlists", + # message_body, + # "noreply@superlists", + # [email], + # ) messages.success( request, "Check your email, we've sent you a link you can use to log in.", @@ -643,31 +652,69 @@ def send_login_email(request): ---- ==== -<1> This should do it! We'll still send an email, - but it won't contain a login URL. +<1> We just comment out the entire send_email block. + + +We rebuild our docker image: -* TODO: aside on moujnting /src/? +[subs="specialcharacters,quotes"] +---- +# check our env var is set +$ *echo $EMAIL_FILE_PATH* +/tmp/superlists-emails +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ + -it superlists* +---- + +// TODO: aside on moujnting /src/? -So let's try it: +And we re-run our test: [subs="specialcharacters,quotes"] ---- -$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails ./src/manage.py test functional_tests.test_login +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ + ./src/manage.py test functional_tests.test_login [...] Ran 1 test in 2.513s OK ---- -==== Testing side-effects is fiddly! -TODO: flesh out explanation +Eh? How did that pass? -eh? what's happening? -It's because we're picking up an old email, which is still a valid token in the DB +=== Testing side-effects is fiddly! + +We've run into an example of the kinds of problems you often encounter +when our tests involve side-effects. + +Let's have a look in our test emails directory: + +[role="skipme"] +[subs="specialcharacters,quotes"] +---- +$ *ls $EMAIL_FILE_PATH* +20241120-153150-262004991022080.log +20241120-153154-262004990980688.log +20241120-153301-272143941669888.log +---- + +Every time we restart the server, it opens a new file, +but only when it first tries to send an email. +Because we've commented out the whole email-sending block, +our test instead picks up on an old email, +which still has a valid url in it, +because the token is still in the database. Let's clear out the db: @@ -700,12 +747,19 @@ ERROR: test_login_using_magic_link (functional_tests.test_login.LoginTest.test_l selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] ---- -OK that's weird, it _does_ still find an email with a magic link in? +OK sure enough, the `wait_to_be_logged_in()` helper is failing, +because now, although we have found an email, its token is invalid. -ah, it's an old one. +Here's another way to make the tests fail: + +[subs="specialcharacters,macros"] +---- +$ pass:[rm $EMAIL_FILE_PATH/*] +---- + +Now when we run the FT: -//// [subs="specialcharacters,quotes"] ---- $ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* @@ -724,18 +778,62 @@ ERROR: test_login_using_magic_link IndexError: list index out of range ---- -That's better! We're not sending any emails, so there's no email file to find. -//// +We see there are no email files, because we're not sending one. -Let's delete all our old emails +Still, this isn't quite satisfactory. +Let's try a different way to make our tests fail, +where we _will_ send an email, but we'll give it the wrong contents: -[subs="specialcharacters,macros"] + +[role="sourcecode"] +.src/accounts/views.py (ch23l006) +==== +[source,python] ---- -$ pass:quotes[*rm $EMAIL_FILE_PATH/*] +def send_login_email(request): + email = request.POST["email"] + token = Token.objects.create(email=email) + url = request.build_absolute_uri( + reverse("login") + "?token=" + str(token.uid), + ) + message_body = f"Use this link to log in:\n\n{url}" + send_mail( + "Your login link for Superlists", + "HAHA NO LOGIN URL FOR U", # <1> + "noreply@superlists", + [email], + ) + messages.success( + request, + "Check your email, we've sent you a link you can use to log in.", + ) + return redirect("/") +---- +==== + +<1> We _do_ send an email, but it won't contain a login URL. + +Let's rebuild again: + +[subs="specialcharacters,quotes"] +---- +# check our env var is set +$ *echo $EMAIL_FILE_PATH* +/tmp/superlists-emails +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ + -it superlists* ---- -And now re rerun the FT: +Now how do our tests look? +[subs="specialcharacters,macros"] ---- $ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] FAIL: test_login_using_magic_link @@ -753,14 +851,35 @@ edith@example.com\nDate: Wed, 13 Nov 2024 18:00:55 -0000\nMessage-ID: U\n-------------------------------------------------------------------------------\n' ---- +That's the error we wanted! Let's revert our temporarily-broken _views.py_, +rebuild, and make sure the tests pass once again. -That's the error we wanted! +[subs="specialcharacters,quotes"] +---- +$ *git stash* +$ *docker build [...]* +# separate terminal +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails [...] +[...] +OK +---- +// todo: aside or title here? + +It may seem like we've done a lot of back-and-forth, +and I could have written the book without this little detour to make the tests fail, +or I could have skipped one of the blind alleys at least, +but I wanted to give you a flavour of the fiddliness involved +in these kinds of tests that involve a lot of side-effects. + + + +//// === Setting Secret Environment Variables on the Server ((("debugging", "server-side", "setting secret environment variables"))) -((("environment variables"))) +((("environment variables"))k) ((("secret values"))) Just as in <>, the place we set environment variables on the server is in the _superlists.env_ file. @@ -774,177 +893,19 @@ Let's change it manually, on the server, for a test: elspeth@server:$ *echo EMAIL_PASSWORD=yoursekritpasswordhere >> ~/superlists.env* elspeth@server:$ *docker restart superlists* ---- - -Now if we rerun our FTs, we see a change: - -[role="against-server small-code"] -[subs="specialcharacters,macros"] ----- -$ pass:quotes[*TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests*] - -[...] -Traceback (most recent call last): - File "...goat-book/functional_tests/test_login.py", line 28, in -test_login_using_magic_link - email = mail.outbox[0] -IndexError: list index out of range - -[...] - -selenium.common.exceptions.NoSuchElementException: Message: Unable to locate -element: Log out ----- - - -The `my_lists` failure is still the same, but we have more information in our login test: -the FT gets further, and the site now looks like it's sending emails correctly -(and the server log no longer shows any errors), -but we can't check the email in the `mail.outbox`... - - -=== Adapting Our FT to Be Able to Test Real Emails via POP3 - -((("debugging", "server-side", "testing POP3 emails", id="DBservemail21"))) -((("Django framework", "sending emails", id="DJFemail21"))) -((("emails, sending from Django", id="email21"))) - -First, we'll need to know, in our FTs, -whether we're running against the staging server or not. -Let's save the `staging_server` variable on `self` in _base.py_: - -[role="sourcecode"] -.src/functional_tests/base.py (ch21l009) -==== -[source,python] ----- - def setUp(self): - self.browser = webdriver.Firefox() - self.test_server = os.environ.get("TEST_SERVER") - if self.test_server: - self.live_server_url = "http://" + self.test_server ----- -==== - -And then we feed through the rest of the changes to the FT that are required -as a result. Firstly, populating a `test_email` variable, differently for -local and staging tests: - - - -[role="sourcecode small-code"] -.src/functional_tests/test_login.py (ch21l011-1) -==== -[source,diff] ----- -@@ -9,7 +9,6 @@ from selenium.webdriver.common.keys import Keys - - from .base import FunctionalTest - --TEST_EMAIL = "edith@example.com" - SUBJECT = "Your login link for Superlists" - - -@@ -34,7 +33,6 @@ class LoginTest(FunctionalTest): - print("getting msg", i) - _, lines, __ = inbox.retr(i) - lines = [l.decode("utf8") for l in lines] -- print(lines) - if f"Subject: {subject}" in lines: - email_id = i - body = "\n".join(lines) -@@ -49,9 +47,14 @@ class LoginTest(FunctionalTest): - # Edith goes to the awesome superlists site - # and notices a "Log in" section in the navbar for the first time - # It's telling her to enter her email address, so she does -+ if self.test_server: -+ test_email = "edith.testuser@yahoo.com" -+ else: -+ test_email = "edith@example.com" -+ - self.browser.get(self.live_server_url) - self.browser.find_element(By.CSS_SELECTOR, "input[name=email]").send_keys( -- TEST_EMAIL, Keys.ENTER -+ test_email, Keys.ENTER - ) ----- -==== - -And then modifications involving using that variable and calling our new helper -function: - -[role="sourcecode small-code"] -.src/functional_tests/test_login.py (ch21l011-2) -==== -[source,diff] ----- -@@ -69,15 +69,13 @@ class LoginTest(FunctionalTest): - ) - - # She checks her email and finds a message -- email = mail.outbox[0] -- self.assertIn(TEST_EMAIL, email.to) -- self.assertEqual(email.subject, SUBJECT) -+ body = self.wait_for_email(test_email, SUBJECT) - -- # It has a URL link in it -- self.assertIn("Use this link to log in", email.body) -- url_search = re.search(r"http://.+/.+$", email.body) -+ # It has a url link in it -+ self.assertIn("Use this link to log in", body) -+ url_search = re.search(r"http://.+/.+$", body) - if not url_search: -- self.fail(f"Could not find url in email body:\n{email.body}") -+ self.fail(f"Could not find url in email body:\n{body}") - url = url_search.group(0) - self.assertIn(self.live_server_url, url) - -@@ -85,10 +83,10 @@ class LoginTest(FunctionalTest): - self.browser.get(url) - - # she is logged in! -- self.wait_to_be_logged_in(email=TEST_EMAIL) -+ self.wait_to_be_logged_in(email=test_email) - - # Now she logs out - self.browser.find_element(By.LINK_TEXT, "Log out").click() - - # She is logged out -- self.wait_to_be_logged_out(email=TEST_EMAIL) -+ self.wait_to_be_logged_out(email=test_email) ----- -==== +//// -And, believe it or not, that'll actually work, and give us an FT -that can actually check for logins that work, involving real emails! +=== Moving on to the next failure +Now if we rerun our full set of FTs, we can move on to the next failure: [role="against-server small-code"] [subs="specialcharacters,macros"] ---- -$ pass:quotes[*TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests.test_login*] -[...] -OK +$ pass:quotes[*TEST_SERVER=localhost:888 python src/manage.py test functional_tests*] ---- -NOTE: I've just hacked this email-checking code together, - and it's currently pretty ugly and brittle - (one common problem is picking up the wrong email from a previous test run). - With some cleanup and a few more retry loops - it could grow into something more reliable. - Alternatively, services like _mailinator.com_ will give you throwaway email addresses - and an API to check them, for a small fee. - ((("", startref="email21"))) - ((("", startref="DJFemail21"))) - ((("", startref="DBservemail21"))) - - -=== Managing the Test Database on Staging - -((("debugging", "server-side", "managing test databases", id="DBservdatabase21"))) -((("staging sites", "managing test databases", id="SSmanag21"))) -((("database testing", "managing test databases", id="DTmanag21"))) -((("sessions, pre-creating"))) Now we can rerun our full FT suite and get to the next failure: our attempt to create pre-authenticated sessions doesn't work, so the "My Lists" test fails: @@ -953,21 +914,29 @@ so the "My Lists" test fails: [subs="specialcharacters,macros"] ---- $ pass:quotes[*TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests*] - +[...] ERROR: test_logged_in_users_lists_are_saved_as_my_lists -(functional_tests.test_my_lists.MyListsTest) +(functional_tests.test_my_lists.MyListsTest.test_logged_in_users_lists_are_saved_as_my_lists) +---------------------------------------------------------------------- +Traceback (most recent call last): + File "...goat-book/src/functional_tests/test_my_lists.py", line 36, in +test_logged_in_users_lists_are_saved_as_my_lists + self.wait_to_be_logged_in(email) + ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^ [...] -selenium.common.exceptions.TimeoutException: Message: Could not find element -with id id_logout. Page text was: -Superlists -Sign in -Start a new To-Do list +selenium.common.exceptions.NoSuchElementException: Message: Unable to locate +element: #id_logout; [...] +[...] + --------------------------------------------------------------------- -Ran 8 tests in 72.742s +Ran 8 tests in 30.087s FAILED (errors=1) ---- + +* TODO: continue rewrites from this point. + It's because our test utility function `create_pre_authenticated_session` only acts on the local database. Let's find out how our tests can manage the database on the server. From ed4e4145d5c83cb306989637e2d24a9a40708aaa Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 27 Nov 2024 21:40:50 +0000 Subject: [PATCH 08/63] callout syntax fixes --- chapter_23_debugging_prod.asciidoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 5c578b68..699886c4 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -276,7 +276,7 @@ Now let's pass that env var thru to our docker container using one more `-e` fla this one fishing the env var out of the shell we're in: -[subs="specialcharacters,quotes"] +[subs="attributes+,quotes"] ---- $ *docker build -t superlists . && docker run \ -p 8888:8888 \ @@ -338,11 +338,11 @@ There are a few different ways we could test this: which give you an email account to send to, and some APIs for checking what mail has been delivered. -2. We could give up on testing email on the server. +3. We could give up on testing email on the server. If we have a minimal smoke test that the server _can_ send emails, then we don't need to test that they are _actually_ delivered. -3. We can use an alternative, fake email backend, +4. We can use an alternative, fake email backend, whereby Django will save the emails to a file on disk for example, and we can inspect them there. @@ -548,7 +548,7 @@ We'll need a couple more minor changes to the FT, to use the helper: Now let's set that file path, and mount it inside our docker container, so that it's available both inside and outside the container: -[subs="specialcharacters,quotes"] +[subs="attributes+,specialcharacters,quotes"] ---- # set a local env var for our path to the emails file $ *export EMAIL_FILE_PATH=/tmp/superlists-emails* From f089b1d574c84d54f11aa3b06979b51658db36f3 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 27 Nov 2024 21:40:55 +0000 Subject: [PATCH 09/63] more in 23 --- chapter_23_debugging_prod.asciidoc | 197 ++++++++++++++++++++++++++--- 1 file changed, 182 insertions(+), 15 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 699886c4..0fecfa8d 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1,5 +1,5 @@ [[chapter_23_debugging_prod]] -== Debugging Production Issues +== Debugging And Testing Production Issues .Warning, Chapter Update in Progress ******************************************************************************* @@ -15,6 +15,14 @@ Popping a few layers off the stack of things we're working on: we have nice wait-for helpers; what were we using them for? Oh yes, waiting to be logged in. And why was that? Ah yes, we had just built a way of pre-authenticating a user. +Let's see how that works against our staging server and Docker. + +In this chapter we'll encounter some situations where the right answer is not always obvious. +We're into the realm of tradeoffs now, which is appropriate for this more advanced part of the book. +My aim is no longer to show you a single, good way to do things, +but instead to give you a toolbox of techniques and strategies, +with some insights into their strengths and weaknesses, +so that you can make decisions on your own. @@ -368,12 +376,23 @@ but you can use any email service you like, as long as it offers POP3 access. You will need to set the `RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. +//// + +TODO: this is the first time we've seen that set -e thing, maybe leave til later + [subs="specialcharacters,quotes"] ---- $ *echo RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere >> .env* $ *set -a; source .env; set +a* ---- +//// + +[subs="specialcharacters,quotes"] +---- +$ *export RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere* +---- + [role="sourcecode skipme"] .src/functional_tests/test_login.py (ch23l001) ==== @@ -424,8 +443,15 @@ TIP: If you _do_ want to test email end-to-end, rather than trying to use POP3 directly. + === Using a Fake Email Backend For Django +Next let's investigate using a filesystem-based email backend. +As we'll see, although it definitely has the advantage +that everything stays local on our own machine +(there are no calls over the internet), +there are quite a few things to watch out for. + Let's say that, if we detect an environment variable `EMAIL_FILE_PATH`, we switch to Django's file-based backend: @@ -780,6 +806,14 @@ IndexError: list index out of range We see there are no email files, because we're not sending one. +NOTE: In this configuration of Docker + `filebase.EmailBackend`, + we now have to manage side effects in two locations: + the database at _src/db.sqlite3_, and the email files in _/tmp_. + What Django used to do for us thanks to LiveServerTestCase + is now all our responsibility, and as you can see, it's hard to get right. + This is a tradeoff to be aware of when writing tests against "real" systems. + + Still, this isn't quite satisfactory. Let's try a different way to make our tests fail, where we _will_ send an email, but we'll give it the wrong contents: @@ -851,7 +885,10 @@ edith@example.com\nDate: Wed, 13 Nov 2024 18:00:55 -0000\nMessage-ID: U\n-------------------------------------------------------------------------------\n' ---- -That's the error we wanted! Let's revert our temporarily-broken _views.py_, +OK good, that's the error we wanted! +I think we can be fairly confident that this testing setup +can genuinely test that emails are sent properly. +Let's revert our temporarily-broken _views.py_, rebuild, and make sure the tests pass once again. [subs="specialcharacters,quotes"] @@ -866,34 +903,164 @@ OK // todo: aside or title here? -It may seem like we've done a lot of back-and-forth, -and I could have written the book without this little detour to make the tests fail, -or I could have skipped one of the blind alleys at least, -but I wanted to give you a flavour of the fiddliness involved -in these kinds of tests that involve a lot of side-effects. +NOTE: It may seem like we've done a lot of back-and-forth, + and I could have written the book without this little detour to make the tests fail, + or I could have skipped one of the blind alleys at least, + but I wanted to give you a flavour of the fiddliness involved + in these kinds of tests that involve a lot of side-effects. +=== Decision Time: Which Test Strategy Will We Keep + +Let's recap our three options: + + +.Testing Strategy Tradeoffs +[cols="1,1,1"] +|======= +| Strategy | Pros | Cons +| End-to-end with POP3 | Maximally realistic, tests the whole system | Slow, fiddly, unreliable +| File-based fake email backend | Faster, more reliable, no network calls, tests end-to-end (albeit with fake components) | Still Fiddly, requires managing db & filesystem side-effects +| Give up on testing email on the server/Docker | Fast, simple | Less confidence that things work "for real" +|======= + +This is a common problem in testing integration with external systems, +how far should we go? How realistic should we make our tests. + +In this case, I'm going to suggest we go for the last option, +which is not to test email sending on the server or in Docker. + +Email itself is a well-understood protocol +(reader, it's been around since _before I was born_, and that's a whiles ago now) +and Django has supported sending email for more than a decade, +so I think we can afford to say, in this case, +that the costs of building testing tools for email outweigh the benefits. + +We can already repro the issue we saw on the server in our Docker image, +so I'm going to suggest we stick to using `mail.outbox` when we're running local tests, +and we configure our FTs to just check that the server (or Docker) _seems_ to be able to send email +(in the sense of "not crashing") and we can skip the bit where we check the email contents in our FT. +Remember, we also have unit tests for the email content! + + +Here's where we can put an early return in the FT: + +[role="sourcecode dofirst-ch23l008"] +.src/functional_tests/test_login.py (ch23l009) +==== +[source,python] +---- + # A message appears telling her an email has been sent + self.wait_for( + lambda: self.assertIn( + "Check your email", + self.browser.find_element(By.CSS_SELECTOR, "body").text, + ) + ) + + if self.against_server: + # Testing real email sending from the server is not worth it. + return + + # She checks her email and finds a message + email = mail.outbox.pop() +---- +==== + +This test will still fail if you don't set `EMAIL_PASSWORD` to a valid value +in Docker or on the server, so that's good enough for now. + +Here's how we populate the `.against_server` attribute: + + +[role="sourcecode"] +.src/functional_tests/base.py (ch23l010) +==== +[source,python] +---- +class FunctionalTest(StaticLiveServerTestCase): + def setUp(self): + self.browser = webdriver.Firefox() + if test_server := os.environ.get("TEST_SERVER"): + self.against_server = True + self.live_server_url = "http://" + test_server + else: + self.against_server = False +---- +==== + + +And you can confirm that the FT will fail if you don't set `EMAIL_PASSWORD` in Docker. + +Now let's see if we can get our FTs to pass against the server: -//// === Setting Secret Environment Variables on the Server -((("debugging", "server-side", "setting secret environment variables"))) ((("environment variables"))k) ((("secret values"))) Just as in <>, the place we set environment variables on the server is in the _superlists.env_ file. -Let's change it manually, on the server, for a test: -* TODO: maybe use ansible straight away? Also, need to discuss secret storage locally. +Let's add it to the template first: -[role="server-commands small-code"] -[subs="specialcharacters,quotes"] + +[role="sourcecode"] +.infra/env.j2 (ch23l011) +==== +[source,python] ---- -elspeth@server:$ *echo EMAIL_PASSWORD=yoursekritpasswordhere >> ~/superlists.env* -elspeth@server:$ *docker restart superlists* +DJANGO_DEBUG_FALSE=1 +DJANGO_SECRET_KEY={{ secret_key }} +DJANGO_ALLOWED_HOST={{ host }} +EMAIL_PASSWORD={{ email_password }} ---- +==== + +and now we add the line to the ansible deploy playbook +that looks up EMAIL_PASSWORD in our local environment: + + +[role="sourcecode dofirst=ch23l012-1"] +.infra/deploy-playbook.yml (ch23l012) +==== +[source,python] +---- + - name: Ensure .env file exists + ansible.builtin.template: + src: env.j2 + dest: ~/superlists.env + force: true # update file if contents changed + vars: + host: "{{ inventory_hostname }}" + secret_key: "{{ lookup('password', '/dev/null length=32 chars=ascii_letters') }}" + email_password: "{{ lookup('env', 'EMAIL_PASSWORD') }}" <1> +---- +==== + +<1> We use another call to `lookup()`, + this one with the `env` parameter, + which is equivalent to `os.environ.get()` in Python. + +// TODO: backport that force=true from ch23l012-1 + + //// +TODO: sidebar on making the secret key only update if changed. + +- name: Check secret key already exists + shell: grep -c "SECRET_KEY" ~/superlists.env || true + register: secret_key_line_count + +- name: add secret key line if not already there + lineinfile: + dest: ~/superlists.env + line: SECRET_KEY={{ secret_key }} + when: secret_key_line_count.stdout == "0" + +or bite the bullet and do it here? +//// + === Moving on to the next failure From f8623656bf1997fde17275a6f9bc4f2e6f18f623 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 27 Nov 2024 23:38:59 +0000 Subject: [PATCH 10/63] sketch out a solution for the db mgmt on server --- chapter_23_debugging_prod.asciidoc | 115 ++++++++++++----------------- 1 file changed, 48 insertions(+), 67 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 0fecfa8d..ed4ab1c8 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1114,7 +1114,7 @@ database on the server. ((("scripts, building standalone"))) To do things on the server, we'll need to build a self-contained script -that can be run from the command line on the server, most probably via Fabric. +that can be run from the command line on the server, most probably via SSH. When trying to build a standalone script that works with Django (i.e., can talk to the database and so on), there are some fiddly issues you need to get right, @@ -1278,49 +1278,63 @@ class FunctionalTest(StaticLiveServerTestCase): ---- ==== + +* TODO: introduce reset_database later, when we get an integrityerror from trying to recreate a user twice. + <1> This will be our function to reset the server database in between each test. We'll write that next, using Fabric. -==== Using Fabric Directly from Python +==== Running Commands on the Server Using the SSH Command -* TODO: rewrite for ansible. +A quick and dirty way of running commands on the server is with SSH and `subprocess`: -((("Fabric", "using directly from Python"))) -Rather than using the `fab` command, Fabric provides an API that lets -you run Fabric server commands directly inline in your Python code. You -just need to let it know the "host string" you're connecting to: [role="sourcecode"] -.src/functional_tests/server_tools.py (ch18l018) +.src/functional_tests/server_tools.py (ch23l02X) ==== [source,python] ---- -from fabric.api import run -from fabric.context_managers import settings, shell_env +import subprocess +USER = "elspeth" -def _get_manage_dot_py(host): - return f"~/sites/{host}/.venv/bin/python ~/sites/{host}/manage.py" + +def _exec_in_container(host, cmd): + print(f"Running {cmd!r} on {host} inside docker container") + process = subprocess.run( + ["ssh", f"{USER}@{host}", f"docker exec superlists {cmd}"], <1> + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + check=False, + ) + result = process.stdout.decode() + if process.returncode != 0: + raise Exception(result) + print(f"Result: {result!r}") + return result.strip() def reset_database(host): - manage_dot_py = _get_manage_dot_py(host) - with settings(host_string=f"elspeth@{host}"): # <1> - run(f"{manage_dot_py} flush --noinput") # <2> + return _exec_in_container( + host, + "/venv/bin/python /src/manage.py flush --noinput", + ) + + +def create_session_on_server(host, email): + return _exec_in_container( + host, + f"/venv/bin/python /src/manage.py create_session {email}", + ) ---- ==== -<1> Here's the context manager that sets the host string, in the form - 'user@server-address' (I've hardcoded my server username, elspeth, so - adjust as necessary). - -<2> Then, once we're inside the context manager, we can just call - Fabric commands as if we're in a fabfile. +<1> We're doing a `docker exec` inside an `ssh`. For creating the session, we have a slightly more complex procedure, @@ -1329,38 +1343,6 @@ the current running server, to be able to generate a session key that's cryptographically valid for the server: -[role="sourcecode small-code"] -.src/functional_tests/server_tools.py (ch18l019) -==== -[source,python] ----- -def _get_server_env_vars(host): - env_lines = run(f"cat ~/sites/{host}/.env").splitlines() # <1> - return dict(l.split("=") for l in env_lines if l) - - -def create_session_on_server(host, email): - manage_dot_py = _get_manage_dot_py(host) - with settings(host_string=f"elspeth@{host}"): - env_vars = _get_server_env_vars(host) - with shell_env(**env_vars): # <2> - session_key = run(f"{manage_dot_py} create_session {email}") # <3> - return session_key.strip() ----- -==== - - -<1> We extract and parse the server's current environment variables from the - _.env_ file... - -<2> In order to use them in another fabric context manager, `shell_env`, - which sets the environment for the next command... - -<3> Which is to run our `create_session` management command, which calls the - same `create_pre_authenticated_session` function, but on the server. - - - ==== Recap: Creating Sessions Locally Versus Staging ((("staging sites", "local vs. staged sessions"))) @@ -1393,10 +1375,10 @@ Perhaps a little ascii-art diagram will help: +-----------------------------------+ +-------------------------------------+ | ^ v | -+----------------------------+ +--------+ +------------------------------+ -| server_tools | --> | fabric | --> | ./manage.py create_session | -| .create_session_on_server | | "run" | | (on server, using .env) | -| (locally) | +--------+ +------------------------------+ ++----------------------------+ +-------------+ +------------------------------+ +| server_tools | --> | ssh and | --> | ./manage.py create_session | +| .create_session_on_server | | docker exec | | (on server, using .env) | +| (locally) | +-------------+ +------------------------------+ +----------------------------+ ---- @@ -1431,20 +1413,17 @@ And now we run the test: ---- $ TEST_SERVER=staging.ottg.co.uk python src/manage.py test \ functional_tests.test_my_lists -[...] -[elspeth@staging.ottg.co.uk] run: -~/sites/staging.ottg.co.uk/.venv/bin/python -~/sites/staging.ottg.co.uk/manage.py flush --noinput -[...] -[elspeth@staging.ottg.co.uk] run: -~/sites/staging.ottg.co.uk/.venv/bin/python -~/sites/staging.ottg.co.uk/manage.py create_session edith@example.com -[...] +Found 1 test(s). +Creating test database for alias 'default'... +System check identified no issues (0 silenced). +Running '/venv/bin/python /src/manage.py create_session edith@example.com' on staging.ottg.co.uk inside docker container +Result: '7n032ogf179t2e7z3olv9ct7b3d4dmas\n' . --------------------------------------------------------------------- -Ran 1 test in 5.701s +Ran 1 test in 4.515s OK +Destroying test database for alias 'default'... ---- Looking good! We can rerun all the tests to make sure... @@ -1464,6 +1443,8 @@ OK Hooray! +* TODO talk about reset_database separately + NOTE: I've shown one way of managing the test database, but you could experiment with others--for example, if you were using MySQL or Postgres, you could open up an SSH tunnel to the server, and use port forwarding to From be05dec2ee06584bfbe178493a7ef0940cecfedb Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 27 Nov 2024 23:39:55 +0000 Subject: [PATCH 11/63] a big todo --- chapter_23_debugging_prod.asciidoc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index ed4ab1c8..8b20eb07 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1102,13 +1102,17 @@ FAILED (errors=1) ---- -* TODO: continue rewrites from this point. It's because our test utility function `create_pre_authenticated_session` only acts on the local database. Let's find out how our tests can manage the database on the server. +=== Repro the error in docker, again + +* TODO: resume here. + + ==== A Django Management Command to Create Sessions From ec96b6a6c5c880762261ad5563b72fd911897adc Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Tue, 10 Dec 2024 21:01:18 +0100 Subject: [PATCH 12/63] fix typos --- chapter_19_spiking_custom_auth.asciidoc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 868fa281..dc8724a0 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -111,7 +111,7 @@ and convince yourself that it really does work. ((("spiking and de-spiking", "branching your VCS"))) ((("Git", "creating branches"))) -This spike is going to be a bit more involved that the last one, +This spike is going to be a bit more involved than the last one, so we'll be a little more rigorous with our version control. Before embarking on a spike it's a good idea to start a new branch, @@ -178,7 +178,7 @@ and a logout link for users who are already authenticated: ((("Django framework", "sending emails", id="DFemail18"))) ((("send_mail function", id="sendmail18"))) ((("emails, sending from Django", id="emails18"))) -The login theory will be something like this: +The login in theory will be something like this: - When someone wants to log in, we generate a unique secret token for them, store it in the database linked to their email, and send it to them. @@ -186,7 +186,7 @@ The login theory will be something like this: - The user then checks their email, which will have a link to a URL that includes that token. -- When they click that link, we check whether the token exists in database, +- When they click that link, we check whether the token exists in the database, and if so, they are logged in as the associated user. // https://docs.djangoproject.com/en/5.0/topics/auth/customizing/ @@ -296,7 +296,7 @@ with our 'base.html' in the real version.) The https://docs.djangoproject.com/en/5.1/topics/email/[django docs on email] explain how `send_mail()` works, as well as how you configure it -by telling Django what email to server to use, +by telling Django what email server to use, and how to authenticate with it. I'm just using my Gmailfootnote:[ Didn't I just spend a whole intro banging on about the privacy implications @@ -542,7 +542,7 @@ $ pass:quotes[*python src/manage.py migrate*] Running migrations: Applying accounts.0002_listuser... OK ---- -/ch18l009-1 +//ch18l009-1 ==== Finishing the Custom Django Auth @@ -562,11 +562,14 @@ Hmm, we can't quite cross off anything yet. Turns out the steps we _thought_ we'd go through aren't quite the same as the steps we're _actually_ going through (this is not uncommon as I'm sure you know). +// CSANAD: I find it vague like this. Maybe it would be helpful to clarify what +// it is that "we are actually going through" and what it was that +// "we thought we'd go through". Still, we're almost there--our last step will combine recognising the token and then actually logging the user in. Once we've done this, -we'll be able to pretty much strike off all the items on our scratchpad: +we'll be able to pretty much strike off all the items on our scratchpad. So here's the view that actually handles the click through from the link in the From d6e89019f3e1a6e5ed31028069b55bf0fde08c90 Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Tue, 10 Dec 2024 21:03:53 +0100 Subject: [PATCH 13/63] add suggestions and notes --- chapter_19_spiking_custom_auth.asciidoc | 37 ++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index dc8724a0..f15db8ae 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -91,6 +91,7 @@ but this is just a fun toy project so let's give it a go. To get this Magic Links project set up, the first thing I did was take a look at existing Python and Django authentication packages, like https://docs.allauth.org/en/latest/[django-allauth] and https://github.com/omab/python-social-auth[python-social-auth], +// CSANAD: python-social-auth looks abandoned but both of them looked overcomplicated for this stage (and besides, it'll be more fun to code our own!). @@ -109,6 +110,11 @@ and convince yourself that it really does work. ==== Starting a Branch for the Spike +// CSANAD: although we introduced to expression "spike" in the 17th chapter, I +// would still mention more explicitly that this is a quick and dirty hacking +// around, which is not production code, and this is the reason we are not +// writing tests just yet - we still need to see what and how to test. + ((("spiking and de-spiking", "branching your VCS"))) ((("Git", "creating branches"))) This spike is going to be a bit more involved than the last one, @@ -182,12 +188,20 @@ The login in theory will be something like this: - When someone wants to log in, we generate a unique secret token for them, store it in the database linked to their email, and send it to them. +// CSANAD: although it should be obvious there is no "database linked to their +// email", still, I suggest a different order of words for clarity: +// "store it linked to their email in the database, (...)" +// or +// "store it linked in the database to their email, (...)" - The user then checks their email, which will have a link to a URL that includes that token. - When they click that link, we check whether the token exists in the database, and if so, they are logged in as the associated user. +// CSANAD: what do you think about rephrasing it a little: +// "...and if so, we authorize/approve the request to authenticate/log in as the +// associated user" // https://docs.djangoproject.com/en/5.0/topics/auth/customizing/ @@ -334,7 +348,9 @@ TIP: If you want to use Gmail as well, ((("", startref="DFemail18"))) ((("", startref="SDemail18"))) - +// CSANAD: I was only able to make it work with 2FA enabled and the +// app-specific password set. If I have time, I'll try again with allowing +// less secure apps too. ==== Another Secret, Another Environment Variable @@ -461,6 +477,14 @@ OK so we've done the first half of "generate and recognise unique tokens": Before we can move on to recognising them and making the login work end-to-end though, we need to sort out user authentication in Django. +// CSANAD: not sure if it's clear for the readers why we need to sort out the +// authn first. Maybe we could just rephrase: +// +// "In order to move on to recognising them and making the login work end-to-end, +// we need to sort out user authentication in Django. +// +// This way we aren't just stating what needs to be done first, but also the +// reason behind it. The first thing we'll need is a user model. I took a dive into the https://docs.djangoproject.com/en/5.0/topics/auth/customizing[Django @@ -523,6 +547,12 @@ class ListUserManager(BaseUserManager): ---- ==== +// CSANAD: ListUserManager has to be defined before ListUser, since its +// reference to ListUser isn't evaluated until `create_user` is called. This is +// not the case the other way around, ListUser's reference to ListUserManager +// is instantiated in the class definition. Maybe we could leave a note about +// this? + No need to worry about what a model manager is at this stage; for now we just need it because we need it, and it works. @@ -608,6 +638,9 @@ def login(request): The `authenticate()` function invokes Django's authentication framework, which we configure using a "custom authentication backend", +// CSANAD: why the quote marks? If it's because this is a reference to the +// official Django docs, we could just make it a link instead to +// https://docs.djangoproject.com/en/5.1/topics/auth/customizing/#writing-an-authentication-backend whose job it is to validate the UID and return a user with the right email. We could have done this stuff directly in the view, @@ -971,6 +1004,8 @@ what information you want to track about users, from explicitly recording first name and last namefootnote:[ A decision which you'll find prominent Django maintainers saying they now regret. Not everyone has a first name and a last name.] +// CSANAD: also, profile-related information has no business in auth-related +// models. to forcing you to use a username. I'm a great believer in not storing information about users unless you absolutely must, From 3c10e6baf2554436386219a2767a7556a3f37b97 Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Tue, 10 Dec 2024 21:04:14 +0100 Subject: [PATCH 14/63] update django docs links from 5.0 to 5.1 --- chapter_19_spiking_custom_auth.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index f15db8ae..8e4b14f3 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -203,7 +203,7 @@ The login in theory will be something like this: // "...and if so, we authorize/approve the request to authenticate/log in as the // associated user" -// https://docs.djangoproject.com/en/5.0/topics/auth/customizing/ +// https://docs.djangoproject.com/en/5.1/topics/auth/customizing/ First let's prep an app for our accounts stuff: @@ -487,7 +487,7 @@ we need to sort out user authentication in Django. // reason behind it. The first thing we'll need is a user model. I took a dive into the -https://docs.djangoproject.com/en/5.0/topics/auth/customizing[Django +https://docs.djangoproject.com/en/5.1/topics/auth/customizing[Django auth documentation] and tried to hack in the simplest possible one: [role="sourcecode"] From 720a97366a2ae87ffc6252eff7787e31657219ab Mon Sep 17 00:00:00 2001 From: David Seddon Date: Thu, 19 Dec 2024 08:36:46 +0000 Subject: [PATCH 15/63] Review of simple form chapter --- chapter_15_simple_form.asciidoc | 40 +++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/chapter_15_simple_form.asciidoc b/chapter_15_simple_form.asciidoc index 4553b56d..e17a81bc 100644 --- a/chapter_15_simple_form.asciidoc +++ b/chapter_15_simple_form.asciidoc @@ -119,7 +119,8 @@ class ItemFormTest(TestCase): self.assertIn('class="form-control form-control-lg"', form.as_p()) ---- ==== - +// DAVID: what about splitting this up into arrange/act/assert and calling .as_p() only once +// (in the act block?) That gives us a fail which justifies some real coding. @@ -167,6 +168,7 @@ And then: ), ---- ==== +// DAVID: the tests still fail. Should the second class here be form-control-lg, as in the test? NOTE: Doing this sort of widget customisation would get tedious if we had a much larger, more complex form. @@ -194,6 +196,9 @@ and rewrite it properly later). Here we're actually using a unit test as a way of experimenting with the forms API. It's actually a pretty good way of learning how it works. + +// DAVID: double "actually". + ******************************************************************************* // SEBASTIAN: Small suggestion - I'd appreciate mentioning breakpoint() for use in test @@ -208,6 +213,8 @@ We want our form to reuse the validation code that we've already defined on our Django provides a special class that can autogenerate a form for a model, called `ModelForm`. As you'll see, it's configured using a special attribute called `Meta`: +// DAVID: would it be more accurate to say "using a special inner class called `Meta`"? + [role="sourcecode"] .src/lists/forms.py ==== @@ -397,6 +404,11 @@ from lists.forms import EMPTY_ITEM_ERROR, ItemForm And the tests still pass: +// DAVID: Might be better to update the test to use the constant at this point, +// too. It's a bit distracting when it happens later in the chapter. +// (Might be worth mentioning that it's good for tests not to require changing if +// you change a piece of configuration like this EMPTY_ITEM_ERROR constant.) + ---- OK ---- @@ -605,6 +617,8 @@ We need to find any occurrences of the old `id` (`id_new_item`) and `name` (`item_text`) and replace them too, with `id_text` and `text`, respectively: +// DAVID: this instruction could be given more clearly. + [role="ignore-errors"] [subs="specialcharacters,quotes"] ---- @@ -1102,6 +1116,7 @@ class ItemValidationTest(FunctionalTest): self.wait_for_row_in_list_table("2: Make tea") ---- ==== +// DAVID: Is it just me or do the earlier chapters have it as 'Buy milk'? <1> Instead of checking for our custom error message, we check using the CSS pseudoselector `:invalid`, @@ -1139,7 +1154,7 @@ doing a refactor that's quite involved...this is the kind of thing that, without tests, would seriously worry me. In fact, I might well have decided that it wasn't worth messing with code that works. -But, because we have a full tests suite, we can delve around, +But, because we have a full test suite, we can delve around, tidying things up, safe in the knowledge that the tests are there to spot any mistakes we make. It just makes it that much likelier that you're going to keep refactoring, @@ -1183,6 +1198,16 @@ so you always need the server side as well if you really care about data integrity; using a form is a nice way of encapsulating that logic. +// DAVID: I wonder if this is slightly missing the point. +// Our functional tests here are very high level - using a headless browser. +// But many developers use the Django test client +// (https://docs.djangoproject.com/en/5.1/topics/testing/tools/#the-test-client) +// for this kind of thing, which doesn't have browser validation built in. +// So we could use that if we really wanted to test the server side validation. +// +// This might be a good opportunity to mention that as an option +// (even if we don't go into detail here) +// and discuss the trade offs of using Selenium versus the test client, versus both. ((("HTML5"))) Also, not all browsers ('cough--Safari--cough') @@ -1204,8 +1229,6 @@ None of us can see the future, and we should concentrate on finding the right solution rather than the time "wasted" on the wrong solution. - - ==== Using the Form's Own Save Method @@ -1283,6 +1306,9 @@ Here's how we can implement a custom save method: ---- ==== +// DAVID: worth showing where this goes in the class, in case someone is thrown by the weird inner class? +// (I know it doesn't actually matter, but might reduce friction.) + The `.instance` attribute on a form represents the database object that is being modified or created. And I only learned that as I was writing this chapter! @@ -1298,6 +1324,7 @@ Ran 23 tests in 0.086s OK ---- +// DAVID: Maybe I took a wrong turn somewhere but I only have 22 tests. Finally, we can refactor our views. `new_list` first: @@ -1408,3 +1435,8 @@ Each test should test one thing:: ((("unit tests", "testing only one thing"))) ((("testing best practices"))) ******************************************************************************* + +// DAVID: Is that `+` a typo? It renders as-is in the html version of the page when I build it locally. + +// DAVID: Second tip: should this specify unit tests? It's certainly not the case for our functional tests. +// Also this tip is missing the 'why', i.e. that it makes it easier to identify the precise bug. From 65e51d0bb80216691534c353f1fa1f07a883dec1 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Thu, 19 Dec 2024 14:33:45 +0000 Subject: [PATCH 16/63] Review of advanced forms chapter --- chapter_16_advanced_forms.asciidoc | 62 ++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/chapter_16_advanced_forms.asciidoc b/chapter_16_advanced_forms.asciidoc index 66338c9c..7d3615dd 100644 --- a/chapter_16_advanced_forms.asciidoc +++ b/chapter_16_advanced_forms.asciidoc @@ -22,7 +22,7 @@ If you want to skip ahead, that's OK too. Make sure you take a quick look at the <>, and the <> at the end. - +// DAVID: should this be 'silliness'? That's the word you use in the aside. === Another FT for Duplicate Items @@ -70,6 +70,9 @@ so it feels right to keep them in the same file. On the other hand, they're logically separate enough that it's practical to keep them in different methods: +// DAVID: This feels a bit hand-wavy. What are we weighing up here? +// For example, does 'signal' matter in functional tests? +// How about speed? [subs="specialcharacters,macros"] ---- @@ -81,6 +84,9 @@ element: .invalid-feedback; [...] Ran 2 tests in 9.613s ---- +// DAVID: Side note: The favicon 404s are getting pretty distracting by this point, I wonder if it would be +// worth fixing / silencing that somehow earlier in the book? + OK, so we know the first of the two tests passes now. Is there a way to run just the failing one, I hear you ask? Why, yes indeed: @@ -101,6 +107,8 @@ element: .invalid-feedback; [...] what we really wanted to do. It's a new test that checks that duplicate items in the same list raise an error: +// DAVID: What do you mean, "here's what we really wanted to do"? + [role="sourcecode"] .src/lists/tests/test_models.py (ch15l002) ==== @@ -115,6 +123,9 @@ def test_duplicate_items_are_invalid(self): ---- ==== +// DAVID: Could consider extracting "bla" into a variable so it's clear we're intending to use +// the same value. + And, while it occurs to us, we add another test to make sure we don't overdo it on our integrity constraints: @@ -136,7 +147,6 @@ def test_CAN_save_same_item_to_different_lists(self): I always like to put a little comment for tests which are checking that a particular use case should 'not' raise an error; otherwise, it can be hard to see what's being tested: - ---- AssertionError: ValidationError not raised ---- @@ -151,6 +161,9 @@ AssertionError: ValidationError not raised // self.fail("Should not raise exception!") // ``` // SEBASTIAN continued: WDYT? +// +// DAVID: Another light-touch approach is to break things up into Arrange-act(-assert) blocks. +// In this case just putting a line above `item.full_clean()` can make it clearer what code is under test. If we want to get it deliberately wrong, we can do this: @@ -242,6 +255,8 @@ implement a constraint which says that an item must be unique for a particular list, or in other words, that `text` and `list` must be unique together: +// DAVID: suggest changing to "models use an inner class called `Meta`". + [role="sourcecode"] .src/lists/models.py (ch15l005) ==== @@ -260,7 +275,7 @@ You might want to take a quick peek at the https://docs.djangoproject.com/en/5.1/ref/models/options/[Django docs on model `Meta` attributes] at this point. - +// DAVID: Should we get them to run the tests to see they pass? [[rewrite-model-test]] ==== Rewriting the Old Model Test @@ -385,6 +400,8 @@ We can do this because the database on our laptop is only used for dev, so the d In <>, we'll deploy our new code to production, and discuss what to do if we run into migrations and data integrity issues at that point. +// DAVID: Shall we get them to run migrations again, since they deleted the database? + Now if we change our duplicates test to do a `.save` instead of a `.full_clean`... // CSANAD: maybe the migrations are a little too long of an insertion between @@ -435,6 +452,8 @@ OK ((("", startref="FTduplicate15")))((("", startref="DITfunctional15")))And now it's time to commit our model-layer changes: +// DAVID: Why are we not committing the change to the functional tests, out of interest? + [role="small-code"] [subs="specialcharacters,macros"] ---- @@ -460,6 +479,9 @@ element: .invalid-feedback; [...] In case you didn't see it as it flew past, the site is 500ing.footnote:[It's showing a server error, code 500. Gotta get with the jargon!] A quick unit test at the view level ought to clear this up: +// DAVID: You've lost me on what thought process you took from that FT failure to writing this test. +// What I would do is actually spin up the site and walk through things manually, to understand the problem better. +// Also, it's not immediately clear what we're testing in this new test. [role="sourcecode"] .src/lists/tests/test_views.py (ch15l009) @@ -564,6 +586,9 @@ custom constructor, which just ignores its `for_list` argument. (I won't show them all, but I'm sure you'll do them, right? Remember, the Goat sees all.) +// DAVID: worth spelling out what you mean here - i.e. go through and fix the exact test failure, +// don't just paste in the whole thing and get it passing in one go. + // JAN: I'm having troubles with lists.models.Item.MultipleObjectsReturned: get() returned more than one Item -- it returned X! lists.tests.test_forms.ItemFormTest.test_form_save/lists.tests.test_forms.ItemFormTest.test_form_save_handles_saving_to_a_list // CSANAD: I did not notice anything like that and the automated tests for the // book also pass here. Can you compare your code against @@ -648,6 +673,12 @@ class ExistingListItemForm(ItemForm): That's a bit of Django voodoo right there, but we basically take the validation error, adjust its error message, and then pass it back into the form. +// DAVID: Too much voodoo IMO. A simpler approach would be to override the +// `clean` method and then use the ORM to check there isn't already an item with that name. +// It would also allow us to avoid mutating the list instance that's passed in, which could +// in theory cause unexpected side effects for calling code doing something weird. +// (You might also want to mention, in passing, that this code assumes that only one person +// is editing a list at a time.) And we're there! A quick commit: @@ -685,6 +716,7 @@ from lists.forms import ( expected_error = escape(DUPLICATE_ITEM_ERROR) ---- ==== +// DAVID: I misread this first and put that line at the end of the test. That brings back our integrity error: @@ -736,7 +768,7 @@ from lists.forms import ExistingListItemForm, ItemForm [...] def view_list(request, list_id): our_list = List.objects.get(id=list_id) - form = ExistingListItemForm(for_list=our_list) + form = ExistingListItemForm(for_list=our_list) // DAVID: remove this line? if request.method == "POST": form = ExistingListItemForm(for_list=our_list, data=request.POST) if form.is_valid(): @@ -758,6 +790,11 @@ TypeError: ItemForm.save() missing 1 required positional argument: 'for_list' Our custom save method from the parent `ItemForm` is no longer needed. Let's make a quick unit test for that: +// DAVID: I didn't clock what was going on here at first. Maybe spell out that it's inheriting +// the custom save method? +// Also, when the tests are failing it's a smell to be writing another test - we should be trying +// to get these passing first, no? + //IDEA: add the form class names here so ppl know which test_form_save and save() [role="sourcecode"] @@ -765,7 +802,7 @@ Let's make a quick unit test for that: ==== [source,python] ---- -class ItemFormTest(TestCase): +class ExistingListItemFormTest(TestCase): [...] def test_form_save(self): mylist = List.objects.create() @@ -794,7 +831,7 @@ NOTE: Personal opinion here: I could have used `super`, but I prefer not to use Python 3's `super()` with no args is awesome to get the immediate parent. Anything else is too error-prone, and I find it ugly besides. YMMV. - +// DAVID: I had to look up what YMMV meant. // SEBASTIAN: IMHO it's actually Django's fault that it handles code reuse using inheritance and methods overriding // Wouldn't do the same thing, but it's your book and your opinion so I shall close my mouth :D @@ -821,6 +858,9 @@ AssertionError: '' != "You've already got this in your list" + You've already got this in your list ---- +// DAVID: I also get: +// AssertionError: '2: Make tea' not found in ['1: Make tea', '2: Purchase milk'] + The error message isn't being displayed because we are not using the Bootstrap classes. Although it would have been nice to minimise hand-written HTML and use Django instead, it seems like we need to bring back our custom @@ -925,6 +965,8 @@ Something seems to be going wrong with the ordering of our list items. Debugging this with an FT is going to be slow, so let's work at the unit test level. +// DAVID: But first, why not just look at what's actually going on by using the application? + We'll add a test that checks that list items are ordered in the order they are inserted. You'll have to forgive me if I jump straight to the right answer, @@ -952,6 +994,7 @@ class ItemModelTest(TestCase): ) ---- ==== +// DAVID: Why not write a test that specifies the functionality we actually want? TIP: FTs are a slow feedback loop. Switch to unit tests when you want to drill down on edge case bugs. @@ -1018,6 +1061,8 @@ i1>, , ] ---- That confirms our suspicion that the ordering was alphabetical. +// DAVID: does it? they are in the same order. + We can fix that in the `class Meta`: [role="sourcecode"] @@ -1071,6 +1116,8 @@ Ran 33 tests in 0.034s OK ---- +// DAVID: The test passes even if we don't add the ordering clause. + ((("", startref="triprep15"))) ((("", startref="queryset15"))) @@ -1108,7 +1155,7 @@ testing views over the last few chapters. *git commit -m "Fix list item ordering, go back to html5 in FT"* ---- ((("", startref="DITlist15"))) - +// DAVID: Any reason not just to do `git commit -am ...` here? === Wrapping Up: What We've Learned About Testing Django @@ -1206,4 +1253,3 @@ them to start using class-based views.((("", startref="FDVduplicate15")))((("", Next we'll try to make our data validation more friendly by using a bit of client-side code. Uh-oh, you know what that means... - From 3e961584ee59d3c996d59f86b14f48deae0eebde Mon Sep 17 00:00:00 2001 From: David Seddon Date: Thu, 19 Dec 2024 16:48:35 +0000 Subject: [PATCH 17/63] Review javascript chapter --- chapter_17_javascript.asciidoc | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/chapter_17_javascript.asciidoc b/chapter_17_javascript.asciidoc index f183cee2..4d1f4883 100644 --- a/chapter_17_javascript.asciidoc +++ b/chapter_17_javascript.asciidoc @@ -24,6 +24,9 @@ once the user started fixing the problem? Just like our nice HTML5 validation errors do? For that we'd need a teeny-tiny bit of JavaScript. +// DAVID: Will readers have started up the application since we added duplicate protection? +// Maybe this is a good opportunity to get them to use the site for a moment and then point out +// that wrinkle. // RITA: In this paragraph, please tell us how we're going to play around with JavaScript and what it'll teach us. I mean, you're going to use Javascript to make it so the duplicate item error message disappears once the user fixes the problem. Python is a delightful language to program in. @@ -180,12 +183,14 @@ AssertionError: True is not false ---- And we can commit this as the first cut of our FT. - +// DAVID: It's been pretty helpful to have your prescribed git commit messages, +// as if you take a wrong turn you can more easily find your way back to a known good +// point in the chapter. So maybe worth providing a snippet for committing here, as usual. TIP: I like to keep helper methods in the FT class that's using them, and only promote them to the base class when they're actually needed elsewhere. It stops the base class from getting too cluttered. YAGNI. - +// DAVID: Worth explaining the acronym? [[js-spike]] === A Quick "Spike" @@ -198,6 +203,8 @@ This will be our first bit of JavaScript. We're also interacting with the Bootstrap CSS framework, which we maybe don't know very well. +// DAVID: 'maybe'? + In <> we saw that you can use a unit test as a way of exploring a new API or tool. Sometimes though, you just want to hack something together @@ -627,6 +634,8 @@ describe("Superlists tests", () => { afterEach(() => { //<1> testDiv.remove(); }); + +}); ---- ==== @@ -647,6 +656,7 @@ but by using `beforeEach` and `afterEach`, I'm making sure that each test gets a completely fresh copy of the html elements involved, so that one test can't affect another. +// DAVID: I feel like I could do with an explanation of how this testing framework works. Let's now have a play with our testing framework, to see if we can find DOM elements and make assertions on whether they are visible. @@ -747,6 +757,8 @@ to avoid filling the chapter with screenshots.) Now that we're acquainted with our JavaScript testing tools, we can switch back to just one test and start to write the real thing: +// DAVID: One test or two? + [role="sourcecode small-code"] .src/lists/static/tests/Spec.js (ch16l012) ==== @@ -825,6 +837,11 @@ textInput.oninput = () => { That doesn't work! We get an _unexpected error_: +// DAVID: In Chrome, I don't see this error (it just fails in the way we expect it). +// I opened up the dev tools and see that the browser is not allowing me to load a +// local file (actually, the first time the OS prompted me and I said no +// without realising what I was stopping). +// In Firefox, however, I do see the same error here. [role="jasmine-output"] [subs="specialcharacters,quotes"] @@ -1044,6 +1061,8 @@ That was a little too easy, even despite that little `initialize()` dance. Let's change our `initialize()` function to deliberately break it. What if we just immediately hide errors? +// DAVID: At this point I open the application up to see if it's behaving as I think. + [role="sourcecode"] .src/lists/static/lists.js (ch16l019) ==== @@ -1405,7 +1424,8 @@ with the right selectors: ---- ==== - +// DAVID: Really we should be using the static tag here. +// https://docs.djangoproject.com/en/5.1/howto/static-files/ Aaaand we run our FT: @@ -1446,9 +1466,10 @@ As the tests flashed past, you may have noticed an unsatisfactory bit of red, still left around our input box. // RITA: Please make sure to conclude this in the spike section so that you can call back to it here. +// DAVID: Personally I think it would be better to edit it so we get it right first time. Wait a minute! We forgot one of the key things we learned in our spike! We don't need to manually hack `style.display=none`, -we can work _with_ the Boostrap framework, +we can work _with_ the Bootstrap framework, and just remove the `.is-invalid` class. OK let's try it in our implementation: @@ -1554,7 +1575,7 @@ const initialize = (inputSelector) => { ---- ==== -Enjoy the way the tests keep passing even though we're giving the function too many arguments! +Enjoy the way the tests keep passing even though we're giving the function too many arguments? Let's strip them down anyway: @@ -1642,7 +1663,7 @@ The modern js onload boilerplate is minimal: ---- ==== - +// DAVID: Worth getting us to commit the code here? === JavaScript Testing in the TDD Cycle From 7df86414335e451296f568ecf852ba5f9e512ce0 Mon Sep 17 00:00:00 2001 From: Jan Giacomelli Date: Fri, 20 Dec 2024 06:58:04 +0100 Subject: [PATCH 18/63] Chapter 18 review --- chapter_18_second_deploy.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/chapter_18_second_deploy.asciidoc b/chapter_18_second_deploy.asciidoc index 02c7db6b..d3599ccb 100644 --- a/chapter_18_second_deploy.asciidoc +++ b/chapter_18_second_deploy.asciidoc @@ -191,6 +191,8 @@ NOTE: Some people don't like to use `push -f` and update an existing tag, and will instead use some kind of version number to tag their releases. Use whatever works for you. +// JAN: I'd advise against using the -f option. If something goes south with the release, it's hard to return to the previous state. I'd suggest simply creating a new tag. Structure that works nicely is ---. For example, goat-book-2024-12-20_10_12_13-86fd578r-new-feature-a-added. If there's any issue, it's easy to check out the latest working tag and redeploy. + And on that note, we can wrap up <>, and move on to the more exciting topics that comprise <>. Can't wait! From 75658ae1166d8e0aedc35e32a9a5366e87d5de39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Buczyn=CC=81ski?= Date: Fri, 20 Dec 2024 07:52:05 +0100 Subject: [PATCH 19/63] Review of chapter 18 --- chapter_18_second_deploy.asciidoc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/chapter_18_second_deploy.asciidoc b/chapter_18_second_deploy.asciidoc index 02c7db6b..b8ed8597 100644 --- a/chapter_18_second_deploy.asciidoc +++ b/chapter_18_second_deploy.asciidoc @@ -138,6 +138,7 @@ $ pass:quotes[*ansible-playbook --user=elspeth -i www.ottg.co.uk, infra/deploy-p Because our migrations introduce a new integrity constraint, you may find that it fails to apply because some existing data violates that constraint. +// SEBASTIAN: Example might be helpful, like adding not null to a column while having some rows with nulls. [role="skipme"] ---- @@ -216,5 +217,3 @@ Look up "continuous delivery" for some background reading. ((("", startref="Dpro17"))) ******************************************************************************* - -// SEBASTIAN: Great summary From 20f443453d1df82926700687b97646e2b4842a3a Mon Sep 17 00:00:00 2001 From: Jan Giacomelli Date: Fri, 20 Dec 2024 08:07:28 +0100 Subject: [PATCH 20/63] Chapter 19 review --- chapter_19_spiking_custom_auth.asciidoc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 868fa281..ecd1a453 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -397,6 +397,7 @@ We've got enough things we need to learn as it is! Let's switch on our new accounts app in _settings.py_: +// JAN: I have "lists.apps.ListsConfig" inside INSTALLED_APPS "lists" [role="sourcecode"] .src/superlists/settings.py (ch18l008-1) @@ -495,6 +496,8 @@ class ListUser(AbstractBaseUser): ---- ==== +// JAN: Do we really need is_staff here? + That's what I call a minimal user model! One field, none of this firstname/lastname/username nonsense, and, pointedly, no password! @@ -883,6 +886,7 @@ selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: input[name=email]; [...] [...] ---- +// JAN: I see the following exception: ModuleNotFoundError: No module named 'accounts', not the selenium one. We should remove all changes from settings.py/urls.py as well The first thing it wants us to do is add an email input element. Bootstrap has some built-in classes for navigation bars, so we'll use them, and include a @@ -1376,7 +1380,7 @@ class Token(models.Model): uid = models.CharField(default=uuid.uuid4, max_length=40) ---- ==== - +// JAN: I think it would be worth mentioning why uuid.uuid4 isn't called. Can be confusing for people when first seeing this And, perhaps with a bit more wrangling of migrations, that should get us to passing tests: From 5c7f484f05f8fdc2d9db869f142eb97c1498d15d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Buczyn=CC=81ski?= Date: Fri, 20 Dec 2024 08:23:55 +0100 Subject: [PATCH 21/63] Review of chapter 19 --- chapter_19_spiking_custom_auth.asciidoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 868fa281..366b0586 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -53,6 +53,7 @@ Oauth? Openid? "Login with Facebook"? Ugh. For me those all have unacceptable creepy overtones; why should Google or Facebook know what sites you're logging into and when? +// SEBASTIAN: As much as I appreciate the history of the chapter, I am not sure if they bring any value to the reader In the first edition I used an experimental project called "Persona", cooked up by a some of the wonderful techno-hippy-idealists at Mozilla, but sadly that project was abandoned. @@ -864,7 +865,7 @@ and reintroduce them one by one in a test-driven way. ==== Reverting Our Spiked Code - +// SEBASTIAN: Oh, this is neat! Remove all the spike code and leave the test behind [subs="specialcharacters,quotes"] ---- From 754adcf25d1fc0cd637bf0b95401b1cd73b80444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Buczyn=CC=81ski?= Date: Fri, 20 Dec 2024 08:57:57 +0100 Subject: [PATCH 22/63] Review of chapter 20 --- chapter_20_mocking_1.asciidoc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index 5aae48e1..3714835e 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -211,6 +211,8 @@ class SendLoginEmailViewTest(TestCase): ---- ==== +// SEBASTIAN: I really like this way of introducing [monkey]patching in tests, congrats + <1> We define a `fake_send_mail` function, which looks like the real `send_mail` function, but all it does is save some information about how it was called, @@ -429,6 +431,9 @@ from django.test import TestCase ---- ==== +// SEBASTIAN: I'd give some hint (maybe visual?) that one should look at the decorator. +// At first, I got lost as I was expecting something to change in the test itself, +// for example mock.patch used as context manager. If you rerun the tests, you'll see they still pass. And since we're always suspicious of any test that still passes after a big change, From 00679a4e665044b458db77ca12fdc88de696a757 Mon Sep 17 00:00:00 2001 From: Jan Giacomelli Date: Fri, 20 Dec 2024 09:25:01 +0100 Subject: [PATCH 23/63] Review chapter 22 --- chapter_22_fixtures_and_wait_decorator.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/chapter_22_fixtures_and_wait_decorator.asciidoc b/chapter_22_fixtures_and_wait_decorator.asciidoc index 188dc65c..7cee8c8c 100644 --- a/chapter_22_fixtures_and_wait_decorator.asciidoc +++ b/chapter_22_fixtures_and_wait_decorator.asciidoc @@ -406,6 +406,7 @@ def wait(fn): #<1> return modified_fn #<2> ---- ==== +// JAN: Why not use functools.wraps here? <1> A decorator is a way of modifying a function; it takes a function as an [keep-together]#argument...# @@ -503,6 +504,7 @@ $ pass:quotes[*python src/manage.py test functional_tests.test_my_lists*] OK ---- +// JAN: This command runs no tests foe me - Ran 0 tests in 0.000s And do you know what's truly satisfying? We can use our `wait` decorator for our `self.wait_for` helper as well! From ed1d0934f393a7e8c2874c1c2b19ec4f721a759a Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 20 Dec 2024 09:14:59 +0000 Subject: [PATCH 24/63] Review of second deploy --- chapter_18_second_deploy.asciidoc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/chapter_18_second_deploy.asciidoc b/chapter_18_second_deploy.asciidoc index 02c7db6b..ec98bb56 100644 --- a/chapter_18_second_deploy.asciidoc +++ b/chapter_18_second_deploy.asciidoc @@ -30,7 +30,11 @@ You can refer back to <> for reminders on Ansible comman === Staging Deploy - +// DAVID: By the time I got to this chapter, I had cancelled my digital ocean subscription +// from the earlier chapter as it was coming out of the free period and I hadn't realised I would need it again. +// Perhaps you should consider mentioning at end of the earlier deployment chapter that they'll need to keep the +// subscription active if they want to follow along with this chapter. Even better, maybe find a way of making +// these chapters closer together? We start with the staging server: @@ -152,6 +156,11 @@ At this point you have two choices: 2. Learn about data migrations. See <>. +// DAVID: The option is not such much to 'learn about data migrations' as to 'perform a data migration'. +// A third option is to manually edit the data on the site. If all we have is a staging site it's +// a reasonable thing to do. I wonder if rather than getting into any of this, though, you could just +// be prescriptive and get them to delete the db. + ==== How to Delete the Database on the Staging Server Here's how you might do option (1): From 7eec016f6b010ed30017613acb52efc4241e365d Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 20 Dec 2024 11:21:25 +0000 Subject: [PATCH 25/63] Review custom auth --- chapter_19_spiking_custom_auth.asciidoc | 62 +++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 5 deletions(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 868fa281..31dc4229 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -201,6 +201,8 @@ $ *cd src && python manage.py startapp accounts* ---- //16l002 +// DAVID: Worth discussing why you chose to make this an app? + And we'll wire up _urls.py_ with at least one URL. In the top-level _superlists/urls.py_... @@ -222,6 +224,8 @@ urlpatterns = [ And in the accounts module's 'urls.py': +// DAVID: Note that this module isn't created by the startapp command, possibly should be rephrased slightly? + [role="sourcecode"] .src/accounts/urls.py (ch18l004) ==== @@ -370,6 +374,12 @@ How are we doing? Let's review where we're at in the process: * _What steps will the user have to go through?_ ***** +// DAVID: In practice would we really cross something off the list like this before giving it a try? +// Might be better to gradually build things up, e.g. write a function to send an email (and check it works). +// Could even use as an excuse to introduce manage.py shell and do it from there? +// Equally with the user interface stuff, maybe starting up the application and having a look at what it looks like? +// Or maybe start with the model and then layer things on top of that. + We'll need a model to store our tokens in the database--they link an email address with a unique ID. Pretty simple: @@ -391,6 +401,8 @@ class Token(models.Model): Yes, I know Django supports UID fields in databases, but I just want to keep things simple for now. +// DAVID: The reader might not know that - make "Yes, I know" something like "Now I happen to know...". +// Also do you definitely mean UID rather than UUID? The point of this spike is about authentication and emails, not optimising database storage. We've got enough things we need to learn as it is! @@ -419,6 +431,9 @@ INSTALLED_APPS = [ We can then do a quick migrations dance to add the token model to the db: +// DAVID: The current working directory is src, maybe we should do a `cd ..` after we +// create the accounts app? + [subs="specialcharacters,macros"] ---- $ pass:quotes[*python src/manage.py makemigrations*] @@ -437,6 +452,9 @@ Running migrations: And at this point, if you actually try the email form in your browser, you'll see we can actually send email! See <> and <> +// DAVID: Maybe we should hand-hold them through this a bit more, reminding them that this is ACTUALLY GOING TO SEND AN EMAIL +// so make sure they spell their email correctly otherwise they'll be spamming someone else. + [[spike-email-sent]] .Looks like we might have sent an email image::images/login-email-sent-page.png["The email sent confirmation page, indicating the server at least thinks it sent an email successfully"] @@ -494,6 +512,7 @@ class ListUser(AbstractBaseUser): return True ---- ==== +// DAVID: Maybe better include the ListUserManager() here too? Or leave it out until we create it? That's what I call a minimal user model! One field, none of this firstname/lastname/username nonsense, @@ -601,7 +620,7 @@ def login(request): return redirect("/") ---- ==== - +// DAVID: Maybe better to do request.GET["uid"] as the exception would be earlier and clearer? The `authenticate()` function invokes Django's authentication framework, which we configure using a "custom authentication backend", @@ -644,6 +663,14 @@ class PasswordlessAuthenticationBackend(BaseBackend): return ListUser.objects.get(email=email) ---- ==== +// DAVID: It would be more idiomatic (and involve fewer SQL queries) to do: +// +// try: +// token = Token.objects.get(uid=uid) +// except Token.DoesNotExist: +// print("no token found", file=sys.stderr) +// return None +// print("got token", file=sys.stderr) Again, lots of debug prints in there, and some duplicated code, @@ -670,6 +697,8 @@ MIDDLEWARE = [ ---- ==== +// DAVID: The place they put these in the settings file doesn't matter so you could just say +// 'put it at the end'. And finally, a logout view: @@ -711,7 +740,7 @@ urlpatterns = [ And we should be all done! Spin up a dev server with `runserver` and try it--believe it or not, -it _acutally_ works: +it _actually_ works: (<>). [[spike-login-worked]] @@ -741,7 +770,7 @@ $ *git status* $ *git add src/accounts* $ *git commit -am "spiked in custom passwordless auth backend"* ---- - +// DAVID: If we're using `-am`, why the manual add of src/accounts? [role="scratchpad"] ***** @@ -827,6 +856,9 @@ class LoginTest(FunctionalTest): self.assertIn(TEST_EMAIL, navbar.text) ---- ==== +// DAVID: Worth mentioning that once we start going near real emails, example.com is +// a special domain intended for this purpose, which you don't need to worry about accidentally +// spamming someone on. <1> Were you worried about how we were going to handle retrieving emails in our tests? Thankfully we can cheat for now! When running tests, Django gives @@ -924,6 +956,10 @@ form for the login email: ---- ==== +// DAVID: Just observing that we are introducing a conceptual dependency here from the lists app on the accounts app. +// It might be worth calling this out explicitly at some point, as it's an architectural choice. +// Also, now we are heading for a multi-app project it might make more sense to put base.html under superlists/templates, +// but this is very much a matter of taste. Now our FT fails because the login form doesn't send us to a real URL yet--you'll see the `Not found:` message in the server output, @@ -949,10 +985,14 @@ $ *cd src && python manage.py startapp accounts* ---- //ch18l021 +// DAVID: probably a good idea to `cd ..` after this command. You could even do a commit just for that, to be able to distinguish the placeholder app files from our modifications. +// DAVID: Tell me what to do Harry! It's helpful to have a prescribed commit +// message in case I take a wrong turn and want to retrace my steps to a known good point. + Next let's rebuild our minimal user model, with tests this time, and see if it turns out neater than it did in the spike. ((("", startref="SDde18"))) @@ -1009,6 +1049,8 @@ class UserModelTest(TestCase): // todo: consider User.objects.create() here, // depending on what we do about IntegrityErrors in chap 13 +// DAVID: It would be good to say what test command to run here, since +// the previous one was just for the lists app. That gives us an expected failure: @@ -1185,7 +1227,8 @@ OK But our model isn't quite as simple as it could be. It has the email field, and also an autogenerated "ID" field as its primary key. We could make it even simpler! - +// DAVID: Maybe spell this out more clearly to the reader that there are actually two fields, +// they might not realise this. ==== Tests as Documentation @@ -1257,6 +1300,10 @@ Migrations for 'accounts': ---- //ch18l034 +// DAVID: Deleting migrations can get readers in a pickle if they have already run migrations locally. +// Might be worth saying we're only doing this because we've just created it, and advise them to delete +// their database if they happen to have run the migration they've just deleted? (Or you can get them +// to run `migrate accounts zero` I think.) ((("", startref="SDminimal18"))) Now both our tests pass: @@ -1271,7 +1318,6 @@ OK It's probably a good time for a commit, too. - === A Token Model to Link Emails with a Unique ID ((("authentication", "token model to link emails", id="SDtoken18"))) @@ -1343,6 +1389,7 @@ class Token(models.Model): uid = models.CharField(max_length=40) ---- ==== +// DAVID: could it confuse people that the max_length is 40 here but 255 in the spike? And this error: @@ -1362,6 +1409,7 @@ specifically designed for generating unique IDs called "uuid" (for "universally unique id"). We can use it like this: +// DAVID: It feels like a strange time to introduce it, seeing as we've already used it in the spike earlier. [role="sourcecode"] .src/accounts/models.py (ch18l040) @@ -1381,6 +1429,9 @@ class Token(models.Model): And, perhaps with a bit more wrangling of migrations, that should get us to passing tests: +// DAVID: The tests will pass without needing to make migrations, +// but next time they make migrations it will include this change too. That doesn't matter that +// much but isn't as clean a git history. Might be simpler just to tell them to makemigrations. [role="dofirst-ch18l041"] [subs="specialcharacters,quotes"] @@ -1400,6 +1451,7 @@ In the next chapter, we'll get into mocking, a key technique for testing external dependencies like email. ((("", startref="SDtoken18"))) +// DAVID: Worth getting them to commit the code? [role="pagebreak-before"] .Exploratory Coding, Spiking, and De-spiking From 0bc03e00721eacb572474a6f2531c67a65160fd4 Mon Sep 17 00:00:00 2001 From: Csanad Beres Date: Fri, 20 Dec 2024 17:25:50 +0100 Subject: [PATCH 26/63] 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 27/63] 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 28/63] 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` From 9a292d83da38efafe2ac743091d26f9605ab82c1 Mon Sep 17 00:00:00 2001 From: David Seddon Date: Fri, 20 Dec 2024 17:08:08 +0000 Subject: [PATCH 29/63] Review of mocking 1 --- chapter_20_mocking_1.asciidoc | 52 +++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/chapter_20_mocking_1.asciidoc b/chapter_20_mocking_1.asciidoc index 5aae48e1..b14fdaab 100644 --- a/chapter_20_mocking_1.asciidoc +++ b/chapter_20_mocking_1.asciidoc @@ -27,6 +27,8 @@ so for the purpose of these unit tests, we'll pretend that this nice Django shor Am I telling you _not_ to use Django's `mail.outbox`? No; use it, it's a neat shortcut. +// DAVID: More than just a shortcut, it's a much better approach than mocking because it's +// a documented, swappable API. But I want to teach mocks because they're a useful general-purpose tool for unit testing external dependencies. You may not always be using Django! @@ -51,7 +53,7 @@ and the use of custom fake objects, are well worth exploring, but they're a more advanced topic. My second book, https://www.cosmicpython.com[Architecture Patterns with Python], -goes in to some detail on these alternatives. +goes into some detail on these alternatives. ******************************************************************************* @@ -85,7 +87,7 @@ class SendLoginEmailViewTest(TestCase): Wire up the `include` in _superlists/urls.py_, plus the `url` in _accounts/urls.py_, and get the test passing with something a bit like this: - +// DAVID: Should we give them a code snippet for this? [role="sourcecode dofirst-ch19l003"] .src/accounts/views.py (ch19l004) @@ -104,6 +106,10 @@ def send_login_email(request): <1> I've added the import of the `send_mail` function as a placeholder for now. +// DAVID: This wrongfooted me as I forgot to include it here (or maybe my IDE linting +// automatically removed the unused import). Might be better to include in the same snippet +// as we use it. + That'll get the test passing: [subs="specialcharacters,quotes"] @@ -166,7 +172,8 @@ def send_login_email(request): return redirect('/') ---- ==== - +// DAVID: The send_mail function probably shouldn't be commented out here. +// Also I'm a bit unclear whether or not I should be typing this in or just reading it. How can we test this, without calling the _real_ `send_mail` function? The answer is that our test can ask Python to swap out the `send_mail` function @@ -221,6 +228,12 @@ class SendLoginEmailViewTest(TestCase): we swap out the real `accounts.views.send_mail` with our fake version—it's as simple as just assigning it. +// DAVID: Maybe would be less confusing to some readers if fake_send_mail was in the module +// scope, rather than as an inner function. + +// DAVID: Might be better to get everything else working, and the test passing, without send_mail at all. +// Then we introduce it, run the test and see it fail because it has some dependencies? Then we can just concentrate on +// the mock bit. It's important to realise that there isn't really anything magical going on here; we're just taking advantage of Python's dynamic nature and scoping rules. @@ -276,6 +289,7 @@ def send_login_email(request): ---- ==== + That gives: [subs="specialcharacters,macros"] @@ -353,6 +367,10 @@ we're able to write the tests and code all the same. ((("", startref="monkey19")))((("", startref="Mmanual19"))) +// DAVID: Worth finding a way to expose the problems with the current approach, in particular: +// - the monkeypatch persists after the test runs. +// - we've had to roll our own logic to see what it was called with. + === The Python Mock Library ((("mocks", "Python Mock library", id="Mpythong19"))) @@ -555,6 +573,7 @@ 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. Let's wire it up in _base.html_: +// DAVID: I'm not sure this is correct, my code already is wired up (albeit to the hard-coded url). [role="sourcecode small-code"] .src/lists/templates/base.html (ch19l012) @@ -564,6 +583,9 @@ Let's wire it up in _base.html_: ---- ==== +// DAVID: There are two forms in base.html, might be worth making it clear which one +// just in case they accidentally do the wrong one. + Does that help? Nope, same error. Why? Because we're not actually displaying a success message after we send the user an email. @@ -638,7 +660,8 @@ def send_login_email(request): return redirect("/") ---- ==== - +// DAVID: Might be worth explaining how the messages framework works - i.e. that if you have a request, +// you can add messages to the session which will be popped off and displayed in the response. [[mocks-tightly-coupled-sidebar]] .Mocks Can Leave You Tightly Coupled to the Implementation @@ -710,6 +733,7 @@ Mocks often end up erring too much on the side of the "how" rather than the "wha TIP: Test should be about behaviour, not implementation. +// DAVID: Say a bit about why? ******************************************************************************* @@ -993,6 +1017,8 @@ def send_login_email(request): // TODO: investigate kwards for reverse() call // reverse("login", token=str(token.uid)) +// DAVID: The unit tests now pass, worth mentioning? + Two more pieces in the puzzle. We need an authentication backend, whose job it will be to examine tokens for validity @@ -1000,7 +1026,14 @@ and then return the corresponding users; then we need to get our login view to actually log users in, if they can authenticate. ((("", startref="Mpythong19")))((("", startref="Pmock19"))) - +// DAVID: Starting this paragraph by talking about authentication backends +// as a 'need' could leave readers behind. Maybe it would be better to point out that the user isn't actually +// logged in, talk about HTTP being stateless and how Django uses sessions +// to get around that, and then say we're going to solve the problem by +// creating an authentication backend. +// (Having said that, it might be much simpler just to use Django's session +// framework directly and set the user in the session. The only snag might be integration +// with the Django Admin but not sure if that's a concern here). === De-spiking Our Custom Authentication Backend @@ -1045,7 +1078,7 @@ Decoding this: ==== 1 if = 1 More Test - +// DAVID: I was confused by how to read this heading for a moment. (I get it now.) A rule of thumb for these sorts of tests: any `if` means an extra test, and any `try/except` means an extra test, so this should be about three tests. @@ -1234,6 +1267,7 @@ And we can handle the final case like this: return None ---- ==== +// DAVID: Consider User.objects.get_or_create. That's turned out neater than our spike! @@ -1248,7 +1282,8 @@ whose job is to retrieve a user based on their unique identifier (the email addr or to return `None` if it can't find one (have another look at <> if you need a reminder). - +// DAVID: I was a bit unclear what you meant here to start with. Maybe worth spelling out/reminding that +// authentication backends need to implement a `get_user` method? Here are a couple of tests for those two requirements: @@ -1383,6 +1418,7 @@ Let's call that a win, and in the next chapter we'll work on integrating it into our login view, and getting our FT passing. +// DAVID: Shall we do a commit? [[mocking-py-sidebar-1]] .On Mocking in Python @@ -1429,5 +1465,5 @@ Mocks can leave you tightly coupled to the implementation:: As we saw in <>, mocks can leave you tightly coupled to your implementation. For that reason, you shouldn't use them unless you have a good reason. - +// DAVID: When rendered, this last point says the same phrase three times in a row. ******************************************************************************* From fd1d7b4145b35515de5dcf4744adcdef81ae326e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Buczyn=CC=81ski?= Date: Fri, 20 Dec 2024 18:32:01 +0100 Subject: [PATCH 30/63] Review of chapter 21 --- chapter_21_mocking_2.asciidoc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/chapter_21_mocking_2.asciidoc b/chapter_21_mocking_2.asciidoc index 49200f13..36460e7d 100644 --- a/chapter_21_mocking_2.asciidoc +++ b/chapter_21_mocking_2.asciidoc @@ -308,6 +308,9 @@ See <>. .Analysing how many tests are needed at different levels image::images/car-factory-illustration.png["An illustration of the car factory, with boxes for each step in the process (build engine, assemble, paint), and descriptions of testing each step separately vs testing them in combination."] +// SEBASTIAN: How about splitting this big image into several smaller ones? At the first encounter, I skipped it only to discover I need to jump up and down to have visualizations of paragraphs below. +// Not a showstopper, tho. + Analysing things in these terms, we think about the inputs and outputs that apply to each type of test, as well as which attributes of the inputs matter, and which don't. @@ -443,6 +446,9 @@ because we've decided to use a particular, specific interface to implement our a which is something we might want to document and verify in our tests, and mocks are one way to enable that. +// SEBASTIAN: I am missing one crucial sentence here - that this Django-provided abstraction IS STABLE, so it's safe to mock it. +// This is part of a public Django API, meaning it's not going anywhere soon or without breaking backwards-compatibility. That would of course be not welcomed by Django users :) + === Starting Again, Test-Driving our Implementation With Mocks @@ -789,6 +795,10 @@ mismatch. Why not use that? For me, the problem with these magic methods is that it's too easy to make a silly typo and end up with a test that always passes: +// SEBASTIAN: actually, this may no longer be valid. +// Since Python 3.5 there is a kwarg to Mock `unsafe` and by default it is true, which makes it fail when we make a typo in the called method. +// also using type hints reduces a chance of making a typo, if only in the test we know the object is Mock. + [role="skipme"] [source,python] ---- @@ -922,7 +932,7 @@ def login(request): See https://www.pythonmorsels.com/using-walrus-operator/[this article] for more explanation. - +// SEBASTIAN: This is not the first occurrence of a walrus operator in this chapter. Is this intended to put an explanation here? This gets our unit test passing: @@ -1164,7 +1174,7 @@ So, I propose we delete the two mocky tests for the happy and unhappy paths, since they are reasonably covered by the non-mocky ones, but I think we can justify keeping the first mocky test, because it adds value by checking that we're doing our authentication -the "right" way, ie by calling into Dango's `auth.authenticate()` function +the "right" way, ie by calling into Django's `auth.authenticate()` function (instead of, eg, instantiating and calling our auth backend ourselves, or even just implementing authentication inline in the view). From a67e03d3d4050a1365d20d50e6e6e02845314deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Buczyn=CC=81ski?= Date: Fri, 20 Dec 2024 18:47:33 +0100 Subject: [PATCH 31/63] Review of chapter 22 --- chapter_22_fixtures_and_wait_decorator.asciidoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/chapter_22_fixtures_and_wait_decorator.asciidoc b/chapter_22_fixtures_and_wait_decorator.asciidoc index 188dc65c..59dafdec 100644 --- a/chapter_22_fixtures_and_wait_decorator.asciidoc +++ b/chapter_22_fixtures_and_wait_decorator.asciidoc @@ -104,6 +104,7 @@ _Being an attempt to explain sessions, cookies, and authentication in Django._ ((("authentication", "cookies and"))) Because HTTP is stateless, +// SEBASTIAN: I remember me, being a junior always puzzled when reading HTTP is stateless. Perhaps an explanation in plain english (periphrasis) would do a better job. It's a second sentence in part that tries to explain something tricky. servers need a way of recognising different clients with _every single request_. IP addresses can be shared, so the usual solution is to give each client a unique session ID, @@ -495,6 +496,8 @@ One of the fun things this can be used for is to make a decorator that changes the arguments of a function. But we won't get into that now. The main thing is that our decorator now works! +// SEBASTIAN: that's actually an awful idea, making it harder to leverage type hints. I wouldn't be giving people such ideas :D + [subs="specialcharacters,macros"] ---- From d05fe5161f42b055e20840872038229bfc041b2f Mon Sep 17 00:00:00 2001 From: David Seddon Date: Mon, 23 Dec 2024 12:47:12 +0000 Subject: [PATCH 32/63] Review of mocks 2 --- chapter_21_mocking_2.asciidoc | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/chapter_21_mocking_2.asciidoc b/chapter_21_mocking_2.asciidoc index 49200f13..3f670719 100644 --- a/chapter_21_mocking_2.asciidoc +++ b/chapter_21_mocking_2.asciidoc @@ -40,7 +40,7 @@ and we'll also have a discussion about how many tests is "enough". We got our auth backend ready in the last chapter, now we need use the backend in our login view. First we add it to _settings.py_: - +// DAVID: Should we do this after writing the failing tests? // todo renumber listings @@ -336,7 +336,8 @@ then we can apply similar reasoning to what we used at the lower level. However many tests we end up with, we need 3 of them to be checking on each colour, and 3 that check that each body type can be painted. - +// DAVID: This section could do with a summary sentence before we change subject, +// it feels like a jolt to go back to the tests without knowing what we're meant to take from it. === Using Mocks to Test Parts of Our System in Isolation @@ -603,6 +604,9 @@ So in our test, we could have done this instead: But you can see how using the `call` helper is nicer. +// DAVID: Might be worth moving the magic assert_called... methods sidebar +// to here. + ******************************************************************************* @@ -1168,6 +1172,9 @@ the "right" way, ie by calling into Dango's `auth.authenticate()` function (instead of, eg, instantiating and calling our auth backend ourselves, or even just implementing authentication inline in the view). +// DAVID: Another approach for all this would be to swap in stub authentication +// backends using Django's override_settings helper. Might be worth mentioning. + TIP: "Test behaviour, not implementation" is a GREAT rule of thumb for tests. But sometimes, the fact that you're using one implementation rather than another really is important. In these cases, a mocky test can be useful. @@ -1211,6 +1218,9 @@ We're just about ready to try our functional test! Let's just make sure our base template shows a different nav bar for logged-in and non–logged-in users (which our FT relies on): +// DAVID: I originally misunderstood that this code snippet was meant to be +// copy-pasted in. Suggest being more explicit. + [role="sourcecode small-code"] .src/lists/templates/base.html (ch20l022) ==== @@ -1292,6 +1302,8 @@ It's because of two things: * Firstly, we need to re-add the email configuration to _settings.py_. +// DAVID: Shouldn't we write a failing test first? If not, why not? + [role="sourcecode"] .src/superlists/settings.py (ch20l023) ==== @@ -1391,7 +1403,7 @@ $ *git commit -m "Custom passwordless auth backend + custom user model"* ((("mocks", "logout link"))) -The last thing we need to do before we call it a day is to test the logout button +The last thing we need to do before we call it a day is to test the logout button. We extend the FT with a couple more steps: [role="sourcecode"] @@ -1509,6 +1521,7 @@ Ran 57 tests in 78.124s OK ---- //54 +// DAVID: Should we get them to `cd ..` back out of src? WARNING: We're nowhere near a truly secure or acceptable login system here. Since this is just an example app for a book, we'll leave it at that, @@ -1533,7 +1546,8 @@ Using mock.return_value:: this usually happens in the "Assert" or "Then" part of your test. It can also be assigned to in the "Arrange" or "Given" part of your test, as a way to say - "we want this mocked-out function to return a particular value" + "we want this mocked-out function to return a particular value". +// DAVID: Could this point be expressed more clearly? Mocks can ensure test isolation and reduce duplication:: You can use mocks to isolate different parts of your code from each other, @@ -1556,7 +1570,7 @@ Mocks can allow you to verify implementation details:: There are alternatives to mocks, but they require rethinking how your code is structured:: In a way, mocks make it "too easy". In other programming languages - that lack Python's dynamic ability to monkepatch things at runtime, + that lack Python's dynamic ability to monkeypatch things at runtime, developers have had to work on alternative ways to test code with dependencies. While these techniques can be more complex, they do force you to think about how your code is structured, @@ -1564,6 +1578,7 @@ There are alternatives to mocks, but they require rethinking how your code is st and to build clean abstractions and interfaces around them. Further discussion is beyond the scope of this book, but check out http://cosmicpython.com[Cosmic Python]. +// DAVID: Suggest removing the word "other". There's a longer worked example of mocks and using them to improve the structure of code in <>. From 91ce4fe66c0656c35150f402620453f81212446a Mon Sep 17 00:00:00 2001 From: David Seddon Date: Mon, 23 Dec 2024 14:02:26 +0000 Subject: [PATCH 33/63] Review of decorator chapter --- chapter_22_fixtures_and_wait_decorator.asciidoc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/chapter_22_fixtures_and_wait_decorator.asciidoc b/chapter_22_fixtures_and_wait_decorator.asciidoc index 188dc65c..60222918 100644 --- a/chapter_22_fixtures_and_wait_decorator.asciidoc +++ b/chapter_22_fixtures_and_wait_decorator.asciidoc @@ -108,11 +108,14 @@ servers need a way of recognising different clients with _every single request_. IP addresses can be shared, so the usual solution is to give each client a unique session ID, which it will store in a cookie and submit with every request. +// DAVID: what is the 'it' in this sentence? The server will store that ID somewhere (by default, in the database), and then it can recognise each request that comes in as being from a particular client. +// DAVID: Is it worth explaining what a cookie is? + If you log in to the site using the dev server, you can actually take a look at your session ID by hand if you like. It's stored under the key `sessionid` by default. @@ -342,7 +345,8 @@ in the `wait_to_be_logged_in/out`. Something like this would look lovely: // TODO: there's a change to the rows= here, backport. - +// DAVID: I didn't realise that I was meant to paste this in yet - +// be more explicit? [role="sourcecode"] .src/functional_tests/base.py (ch20l005) @@ -591,4 +595,3 @@ Avoid JSON fixtures:: ((("fixtures", "JSON fixtures"))) ******************************************************************************* - From b2f18ccc7c4caef8da9df64f0ecc12f6e047c2f5 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 8 Jan 2025 15:50:10 +0000 Subject: [PATCH 34/63] todo --- chapter_23_debugging_prod.asciidoc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 8b20eb07..46194440 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1053,10 +1053,12 @@ TODO: sidebar on making the secret key only update if changed. register: secret_key_line_count - name: add secret key line if not already there + when: secret_key_line_count.stdout == "0" lineinfile: dest: ~/superlists.env line: SECRET_KEY={{ secret_key }} - when: secret_key_line_count.stdout == "0" + vars: + secret_key: "{{ lookup('password', '/dev/null length=32 chars=ascii_letters') }}" or bite the bullet and do it here? //// @@ -1070,7 +1072,7 @@ Now if we rerun our full set of FTs, we can move on to the next failure: [role="against-server small-code"] [subs="specialcharacters,macros"] ---- -$ pass:quotes[*TEST_SERVER=localhost:888 python src/manage.py test functional_tests*] +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] ---- Now we can rerun our full FT suite and get to the next failure: @@ -1103,20 +1105,18 @@ FAILED (errors=1) -It's because our test utility function `create_pre_authenticated_session` only -acts on the local database. Let's find out how our tests can manage the -database on the server. - - -=== Repro the error in docker, again - -* TODO: resume here. +It's because our test utility function `create_pre_authenticated_session()` +only acts on the local database. +Let's find out how our tests can manage the database on the server. ==== A Django Management Command to Create Sessions +* TODO: resume here. + ((("scripts, building standalone"))) +We need a way to make changes to the database inside docker, or on the server. To do things on the server, we'll need to build a self-contained script that can be run from the command line on the server, most probably via SSH. From 3c6dab91e8d23fb1a197bc4f949a7899b5a6f483 Mon Sep 17 00:00:00 2001 From: Harry Date: Thu, 9 Jan 2025 12:10:40 +0000 Subject: [PATCH 35/63] little bits --- chapter_23_debugging_prod.asciidoc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 46194440..cb14e9be 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -318,13 +318,14 @@ test_login_using_magic_link IndexError: pop from empty list ---- -Aha! The tests get a little further. +Well, not a pass, but the tests do get a little further. It looks like our server _can_ now send emails, (and the docker log no longer shows any errors), they're just not appearing in `mail.outbox`. The reason is that `mail.outbox` is a local, in-memory variable in Django, -so that's only going to work when our tests and our server are running in the same process, like they do with unit tests or with `LiveServerTestCase` FTs. +so that's only going to work when our tests and our server are running in the same process, +like they do with unit tests or with `LiveServerTestCase` FTs. When we run against another process, be it Docker or an actual server, we can't access the same `mail.outbox` variable. @@ -346,21 +347,21 @@ There are a few different ways we could test this: which give you an email account to send to, and some APIs for checking what mail has been delivered. -3. We could give up on testing email on the server. - If we have a minimal smoke test that the server _can_ send emails, - then we don't need to test that they are _actually_ delivered. - -4. We can use an alternative, fake email backend, +3. We can use an alternative, fake email backend, whereby Django will save the emails to a file on disk for example, and we can inspect them there. +4. Or we could give up on testing email on the server. + If we have a minimal smoke test that the server _can_ send emails, + then we don't need to test that they are _actually_ delivered. + I'm not going to explore option 2 in this book, since it involves a commercial service and I don't want to endorse one, but that's not to say it's a bad option. Especially since they have free plans these days! -But let's explore the other three options (1, 2 and 4) and their pros+cons. +But let's take a quick look at options 1 and 3 the and their pros+cons. === How to Test Email End-To-End with POP3 @@ -377,7 +378,6 @@ You will need to set the `RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. //// - TODO: this is the first time we've seen that set -e thing, maybe leave til later [subs="specialcharacters,quotes"] From 6c6c895a898f8da9a7e36f7446fc6122838f123d Mon Sep 17 00:00:00 2001 From: Harry Date: Thu, 9 Jan 2025 12:10:50 +0000 Subject: [PATCH 36/63] whitespace stuff in 24 --- chapter_24_outside_in.asciidoc | 52 ++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/chapter_24_outside_in.asciidoc b/chapter_24_outside_in.asciidoc index 65302680..02374d87 100644 --- a/chapter_24_outside_in.asciidoc +++ b/chapter_24_outside_in.asciidoc @@ -40,8 +40,9 @@ such as views and templates, taking advantage of the new attribute, and then finally add URL routing to point to the new view. It feels comfortable because it means you're never working on a bit of code -that is dependent on something that hasn't yet been implemented. Each bit of -work on the inside is a solid foundation on which to build the next layer out. +that is dependent on something that hasn't yet been implemented. +Each bit of work on the inside is a solid foundation +on which to build the next layer out. But working inside-out like this also has some weaknesses. @@ -50,28 +51,31 @@ But working inside-out like this also has some weaknesses. ((("Outside-In TDD", "vs. inside-out", secondary-sortas="inside-out"))) ((("inside-out TDD"))) -The most obvious problem with inside-out is that it requires us to stray from a -TDD workflow. Our functional test's first failure might be due to missing URL -routing, but we decide to ignore that and go off adding attributes to our -database model objects instead. - -We might have ideas in our head about the new desired behaviour of our inner -layers like database models, and often these ideas will be pretty good, but -they are actually just speculation about what's really required, because -we haven't yet built the outer layers that will use them. - -One problem that can result is to build inner components that are more -general or more capable than we actually need, which is a waste of time, -and an added source of complexity for your project. Another common problem -is that you create inner components with an API which is convenient for their -own internal design, but which later turns out to be inappropriate for the -calls your outer layers would like to make...worse still, you might end up -with inner components which, you later realise, don't actually solve the -problem that your outer layers need solved. - -In contrast, working outside-in allows you to use each layer to imagine the -most convenient API you could want from the layer beneath it. Let's see it in -action. +The most obvious problem with inside-out is that +it requires us to stray from a TDD workflow. +Our functional test's first failure might be due to missing URL routing, +but we decide to ignore that +and go off adding attributes to our database model objects instead. + +We might have ideas in our head +about the new desired behaviour of our inner layers like database models, +and often these ideas will be pretty good, +but they are actually just speculation about what's really required, +because we haven't yet built the outer layers that will use them. + +One problem that can result is to build inner components that are more general +or more capable than we actually need, which is a waste of time, +and an added source of complexity for your project. +Another common problem is that you create inner components +with an API which is convenient for their own internal design, +but which later turns out to be inappropriate +for the calls your outer layers would like to make...worse still, +you might end up with inner components which, you later realise, +don't actually solve the problem that your outer layers need solved. + +In contrast, working outside-in allows you to use each layer +to imagine the most convenient API you could want from the layer beneath it. +Let's see it in action. === The FT for "My Lists" From 6f7c6cbec505b7dde6035dc8fe3e116ede054d46 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 20 Jan 2025 11:42:12 +0000 Subject: [PATCH 37/63] just faffning --- chapter_23_debugging_prod.asciidoc | 27 +++++++-------------- source/chapter_23_debugging_prod/superlists | 2 +- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index cb14e9be..a2848f38 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -1,13 +1,14 @@ [[chapter_23_debugging_prod]] == Debugging And Testing Production Issues -.Warning, Chapter Update in Progress +.Warning, Chapter Recently Updated ******************************************************************************* -🚧 Warning, Chapter update for 3e in progress. +This chapter has been recently updated to Django 5, Ansible+Docker, etc. -This chapter has only been partially updated to Django 5, Ansible+Docker, etc. +It all works on my machine, as they say! +Let me know if you run into anything strange. +Feedback and suggestions of any kind appreciated, as always. -Following along may be tricky, but I hope to have it in better shape soon! ******************************************************************************* @@ -17,6 +18,7 @@ Oh yes, waiting to be logged in. And why was that? Ah yes, we had just built a way of pre-authenticating a user. Let's see how that works against our staging server and Docker. +// TODO revise this? In this chapter we'll encounter some situations where the right answer is not always obvious. We're into the realm of tradeoffs now, which is appropriate for this more advanced part of the book. My aim is no longer to show you a single, good way to do things, @@ -331,7 +333,7 @@ When we run against another process, be it Docker or an actual server, we can't access the same `mail.outbox` variable. We need another technique if we want to actually inspect the emails -that the server sends, in our tests against Docker or the server. +that the server sends, in our tests against Docker or staging. === Deciding How to Test "Real" Email Sending @@ -377,17 +379,6 @@ but you can use any email service you like, as long as it offers POP3 access. You will need to set the `RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. -//// -TODO: this is the first time we've seen that set -e thing, maybe leave til later - -[subs="specialcharacters,quotes"] ----- -$ *echo RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere >> .env* -$ *set -a; source .env; set +a* ----- - -//// - [subs="specialcharacters,quotes"] ---- $ *export RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere* @@ -1113,10 +1104,10 @@ Let's find out how our tests can manage the database on the server. ==== A Django Management Command to Create Sessions -* TODO: resume here. +TODO resume here ((("scripts, building standalone"))) -We need a way to make changes to the database inside docker, or on the server. +We need a way to make changes to the database inside Docker, or on the server. To do things on the server, we'll need to build a self-contained script that can be run from the command line on the server, most probably via SSH. diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 7b206320..e87f16e1 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 7b2063201e53ca3b701e631380ff3fdf58a87301 +Subproject commit e87f16e1cd873a0996268f5016b7cb80b21351f4 From c982d53c4dfd0a485565b69f4cf84b138a9bf403 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 22 Jan 2025 16:23:28 +0000 Subject: [PATCH 38/63] ok made it thru to the end of 23 --- chapter_23_debugging_prod.asciidoc | 317 +++++++++++--------- source/chapter_23_debugging_prod/superlists | 2 +- todos.txt | 8 + 3 files changed, 192 insertions(+), 135 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index a2848f38..c77040aa 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -896,7 +896,7 @@ OK NOTE: It may seem like we've done a lot of back-and-forth, and I could have written the book without this little detour to make the tests fail, - or I could have skipped one of the blind alleys at least, + or I could have skipped one of the weird bugs at least, but I wanted to give you a flavour of the fiddliness involved in these kinds of tests that involve a lot of side-effects. @@ -1104,22 +1104,22 @@ Let's find out how our tests can manage the database on the server. ==== A Django Management Command to Create Sessions -TODO resume here +We need a way to make changes to the database inside Docker, or on the server. +Essentially we want to run some code outside the context of the tests +(and the test database) and in the context of the server and its database. ((("scripts, building standalone"))) -We need a way to make changes to the database inside Docker, or on the server. -To do things on the server, we'll need to build a self-contained script -that can be run from the command line on the server, most probably via SSH. +When trying to build a standalone script that works with Django +(i.e., can talk to the database and so on), +there are some fiddly issues you need to get right, +like setting the `DJANGO_SETTINGS_MODULE` environment variable, +and setting `sys.path` correctly. -When trying to build a standalone script that works with Django (i.e., can talk -to the database and so on), there are some fiddly issues you need to get right, -like setting the `DJANGO_SETTINGS_MODULE` environment variable, and getting -`sys.path` correctly. Instead of messing about with all that, Django lets you create your own "management commands" (commands you can run with `python manage.py`), which will do all that path mangling for you. They live in a folder called -'management/commands' inside your apps: +_management/commands_ inside your apps: [subs=""] ---- @@ -1127,14 +1127,13 @@ $ mkdir -p src/functional_tests/management/commands $ touch src/functional_tests/management/__init__.py $ touch src/functional_tests/management/commands/__init__.py ---- -//ch21l012-1 The boilerplate in a management command is a class that inherits from `django.core.management.BaseCommand`, and that defines a method called `handle`: [role="sourcecode"] -.src/functional_tests/management/commands/create_session.py (ch21l012) +.src/functional_tests/management/commands/create_session.py (ch23l014) ==== [source,python] ---- @@ -1182,7 +1181,7 @@ for it to recognise it as a real app that might have management commands as well as tests: [role="sourcecode"] -.src/superlists/settings.py (ch21l014) +.src/superlists/settings.py (ch23l015) ==== [source,python] ---- @@ -1205,9 +1204,10 @@ $ pass:quotes[*python src/manage.py create_session a@b.com*] qnslckvp2aga7tm6xuivyb0ob1akzzwl ---- -NOTE: If you see an error saying the `auth_user` table is missing, you may need - to run `manage.py migrate`. In case that doesn't work, delete the - _db.sqlite3_ file and run +migrate+ again, to get a clean slate. +NOTE: If you see an error saying the `auth_user` table is missing, + you may need to run `manage.py migrate`. + In case that doesn't work, delete the _db.sqlite3_ file + and run `migrate` again, to get a clean slate. ==== Getting the FT to Run the Management Command on the Server @@ -1217,15 +1217,15 @@ when we're on the local server, and make it run the management command on the staging server if we're on that: [role="sourcecode"] -.src/functional_tests/test_my_lists.py (ch21l016) +.src/functional_tests/test_my_lists.py (ch23l016) ==== [source,python] ---- from django.conf import settings from .base import FunctionalTest +from .container_commands import create_session_on_server from .management.commands.create_session import create_pre_authenticated_session -from .server_tools import create_session_on_server class MyListsTest(FunctionalTest): @@ -1251,58 +1251,64 @@ class MyListsTest(FunctionalTest): ==== -Let's also tweak _base.py_, to gather a bit more information -when we populate `self.test_server`: + + + +==== Running Commands Using Docker Exec and (optionally) SSH + + +You may remember `docker exec` from <>, it lets us run +commands inside a running Docker container. +That's fine for when we're running against the local Docker, +but when we're against the server, we need to SSH in first. + +There's a bit of plumbing here, but I've tried to break things down into small chunks: [role="sourcecode"] -.src/functional_tests/base.py (ch21l017) +.src/functional_tests/container_commands.py (ch23l018) ==== [source,python] ---- -from .server_tools import reset_database #<1> -[...] - -class FunctionalTest(StaticLiveServerTestCase): - def setUp(self): - self.browser = webdriver.Firefox() - self.test_server = os.environ.get("TEST_SERVER") - if self.test_server: - self.live_server_url = "http://" + self.test_server - reset_database(self.test_server) #<1> ----- -==== - - -* TODO: introduce reset_database later, when we get an integrityerror from trying to recreate a user twice. +import subprocess -<1> This will be our function to reset the server database in between each - test. We'll write that next, using Fabric. +USER = "elspeth" +def create_session_on_server(host, email): + return _exec_in_container( + host, ["/venv/bin/python", "/src/manage.py", "create_session", email] # <1> + ) -==== Running Commands on the Server Using the SSH Command +def _exec_in_container(host, commands): + if "localhost" in host: # <2> + return _exec_in_container_locally(commands) + else: + return _exec_in_container_on_server(host, commands) -A quick and dirty way of running commands on the server is with SSH and `subprocess`: +def _exec_in_container_locally(commands): + print(f"Running {commands} on inside local docker container") + return _run_commands(["docker", "exec", _get_container_id()] + commands) # <3> +def _exec_in_container_on_server(host, commands): + print(f"Running {commands!r} on {host} inside docker container") + return _run_commands( + ["ssh", f"{USER}@{host}", "docker", "exec", "superlists"] + commands # <4> + ) -[role="sourcecode"] -.src/functional_tests/server_tools.py (ch23l02X) -==== -[source,python] ----- -import subprocess -USER = "elspeth" +def _get_container_id(): + return subprocess.check_output( # <5> + ["docker", "ps", "-q", "--filter", "ancestor=superlists"] # <3> + ).strip() -def _exec_in_container(host, cmd): - print(f"Running {cmd!r} on {host} inside docker container") - process = subprocess.run( - ["ssh", f"{USER}@{host}", f"docker exec superlists {cmd}"], <1> +def _run_commands(commands): + process = subprocess.run( # <5> + commands, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=False, @@ -1312,30 +1318,27 @@ def _exec_in_container(host, cmd): raise Exception(result) print(f"Result: {result!r}") return result.strip() - - -def reset_database(host): - return _exec_in_container( - host, - "/venv/bin/python /src/manage.py flush --noinput", - ) - - -def create_session_on_server(host, email): - return _exec_in_container( - host, - f"/venv/bin/python /src/manage.py create_session {email}", - ) ---- ==== -<1> We're doing a `docker exec` inside an `ssh`. +<1> We invoke our management command with the path to the virtualenv python, + the `create_session` command name, and pass in the email we want to create a session for + +<2> We dispatch to two slightly different ways of running a command inside a container, + with the assumption that a host that's on "localhost" is a local Docker container, + and the others are on the staging server. + +<3> To run a command on the local Docker container, we're going to use `docker exec`, + and we have a little extra hop first to get the correct container ID. +<4> To run a command on the Docker container that's on the staging server, + we still use `docker exec`, but we do it inside an SSH session. + In this case we don't need the container ID, because the container is always named "superlists'. -For creating the session, we have a slightly more complex procedure, -because we need to extract the `SECRET_KEY` and other env vars from -the current running server, to be able to generate a session key that's -cryptographically valid for the server: +<5> Finally we use Python's `subprocess` module to actually run a command. + You can see a couple of different ways of running it here, + which differ based on how we're handing errors and output; + the details don't matter too much. ==== Recap: Creating Sessions Locally Versus Staging @@ -1350,36 +1353,75 @@ Perhaps a little ascii-art diagram will help: [role="skipme small-code"] ---- - +-----------------------------------+ +-------------------------------------+ | MyListsTest | --> | .management.commands.create_session | | .create_pre_authenticated_session | | .create_pre_authenticated_session | | (locally) | | (locally) | +-----------------------------------+ +-------------------------------------+ - ---- -===== Against staging: + +===== Against Docker locally: [role="skipme small-code"] ---- -+-----------------------------------+ +-------------------------------------+ -| MyListsTest | | .management.commands.create_session | -| .create_pre_authenticated_session | | .create_pre_authenticated_session | -| (locally) | | (on server) | -+-----------------------------------+ +-------------------------------------+ - | ^ - v | -+----------------------------+ +-------------+ +------------------------------+ -| server_tools | --> | ssh and | --> | ./manage.py create_session | -| .create_session_on_server | | docker exec | | (on server, using .env) | -| (locally) | +-------------+ +------------------------------+ ++-----------------------------------+ +-------------------------------------+ +| MyListsTest | | .management.commands.create_session | +| .create_pre_authenticated_session | | .create_pre_authenticated_session | +| (locally) | | (in Docker) | ++-----------------------------------+ +-------------------------------------+ + | ^ + v | ++----------------------------+ +-------------+ +----------------------------+ +| server_tools | --> | docker exec | --> | ./manage.py create_session | +| .create_session_on_server | +-------------+ | (in Docker) | +| (locally) | +----------------------------+ +----------------------------+ +---- + +===== Against Docker locally: +[role="skipme small-code"] ---- ++-----------------------------------+ +-------------------------------------+ +| MyListsTest | | .management.commands.create_session | +| .create_pre_authenticated_session | | .create_pre_authenticated_session | +| (locally) | | (on server) | ++-----------------------------------+ +-------------------------------------+ + | ^ + v | + ++----------------------------+ +-----+ +-------------+ +------------------------------+ +| server_tools | --> | ssh | -> | docker exec | --> | ./manage.py create_session | +| .create_session_on_server | +-----+ +-------------+ | (on server) | +| (locally) | +------------------------------+ ++----------------------------+ + + + +.An Alternative For Managing Test Database Content: Talking Directly to the DB +********************************************************************** +An alternative way of managing database content inside Docker, +or on a server, would be to talk directly to the DB + +Since we're using SQLite, that involves writing to the file directly, +This can be fiddly to get right, because when we're running inside Django's +test runner, Django takes over test database creation, +so you end up having to write raw SQL and managing your connections to the database directly. + +There are also some tricky interactions with the filesystem mounts and Docker, +as well as needing to have the SECRET_KEY env var set to the same value as on the server. + +If we were using a "classic" database server like Postgres or MySQL, +we'd be able to talk directly to the database over its port, +and that's an approach I've used successfully in the past (see eg https://www.cosmicpython.com/book/chapter_02_repository.html#_inverting_the_dependency_orm_depends_on_model) +but it's still fiddly to get right and usually requires writing your own SQL. +********************************************************************** -In any case, let's see if it works. First, locally, to check that we didn't -break anything: +=== Testing the Management Command + +In any case, let's see if it works. +First, locally, to check that we didn't break anything: [role="dofirst-ch21l022"] @@ -1438,19 +1480,64 @@ OK Hooray! -* TODO talk about reset_database separately -NOTE: I've shown one way of managing the test database, but you could - experiment with others--for example, if you were using MySQL or Postgres, - you could open up an SSH tunnel to the server, and use port forwarding to - talk to the database directly. You could then amend `settings.DATABASES` - during FTs to talk to the tunnelled port. You'd still need some way of - pulling in the staging server environment variables though.((("", startref="DBservdatabase21")))((("", startref="SSmanag21")))((("", startref="DTmanag21"))) +=== Test Database Cleanup + +If you try running the tests twice, you'll run into this error: + +[subs="specialcharacters,quotes"] +---- +django.db.utils.IntegrityError: UNIQUE constraint failed: accounts_user.email +---- + +It's because the user we created the first time we ran the tests is still in the database. +When we're running against Django's test database, Django cleans up for us. +Let's try and emulate that when we're running against a real database: + + + + +[role="sourcecode"] +.src/functional_tests/container_commands.py (ch23l019) +==== +[source,python] +---- +def reset_database(host): + return _exec_in_container( + host, ["/venv/bin/python", "/src/manage.py", "flush", "--noinput"] + ) +---- +==== + + +And let's add the call to `reset_database()` in our base test `setUp()` method: + + +[role="sourcecode"] +.src/functional_tests/base.py (ch23l020) +==== +[source,python] +---- +from .server_tools import reset_database #<1> +[...] + +class FunctionalTest(StaticLiveServerTestCase): + def setUp(self): + self.browser = webdriver.Firefox() + self.test_server = os.environ.get("TEST_SERVER") + if self.test_server: + self.live_server_url = "http://" + self.test_server + reset_database(self.test_server) +---- +==== + + +If you try to run your tests again, you'll find they pass happily. [role="pagebreak-before less_space"] -.Warning: Be Careful Not to Run Test Code Against the Live Server +.Warning: Be Careful Not to Run Test Code Against the Production Server! ******************************************************************************* ((("database testing", "safeguarding production databases"))) ((("production databases"))) @@ -1471,44 +1558,6 @@ in <>. LFMF. ******************************************************************************* -=== Updating our Deploy Script - -* TODO: ansible. - -((("debugging", "server-side", "baking in logging code"))) -Before we finish, let's update our deployment fabfile so that it can -automatically add the `EMAIL_PASSWORD` to the _.env_ file on the server: - - -[role="sourcecode skipme"] -.src/deploy_tools/fabfile.py (ch18l021) -==== -[source,python] ----- -import os -[...] - - -def _create_or_update_dotenv(): - append(".env", "DJANGO_DEBUG_FALSE=y") - append(".env", f"SITENAME={env.host}") - current_contents = run("cat .env") - if "DJANGO_SECRET_KEY" not in current_contents: - new_secret = "".join( - random.SystemRandom().choices("abcdefghijklmnopqrstuvwxyz0123456789", k=50) - ) - append(".env", f"DJANGO_SECRET_KEY={new_secret}") - email_password = os.environ["EMAIL_PASSWORD"] # <1> - append(".env", f"EMAIL_PASSWORD={email_password}") # <1> ----- -==== - -<1> We just add two lines at the end of the script which will essentially - copy the local `EMAIL_PASSWORD` environment variable up to the server's - _.env_ file. - - - === Wrap-Up Actually getting your new code up and running on a server always tends to @@ -1532,8 +1581,8 @@ the ability to save their lists on a "My Lists" page. Fixtures also have to work remotely:: `LiveServerTestCase` makes it easy to interact with the test database using the Django ORM for tests running locally. Interacting with the - database on the staging server is not so straightforward. One solution - is Fabric and Django management commands, as I've shown, but you should + database inside Docker is not so straightforward. One solution + is `docker exec` and Django management commands, as I've shown, but you should explore what works for you--SSH tunnels, for example. ((("fixtures", "staging and"))) ((("staging sites", "fixtures and"))) diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index e87f16e1..1b0004f4 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit e87f16e1cd873a0996268f5016b7cb80b21351f4 +Subproject commit 1b0004f4943d56d401551c1ff941b9cb025e78ab diff --git a/todos.txt b/todos.txt index 51f4d719..5d349653 100644 --- a/todos.txt +++ b/todos.txt @@ -3,6 +3,14 @@ # Later +## switch to postgres + +- do it in deploy chaps +- production-readiness really +- install it locally? +- or, put it in a docker container? +- docker-compose?? + - consider splitting 23 into two chapters - figure out how to delete at least one FT? - spike chap: start with test of login view. From 50e47694e6cd31a4d325863cc6cc8e54d127d119 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 22 Jan 2025 16:52:04 +0000 Subject: [PATCH 39/63] tweaks for tests --- chapter_23_debugging_prod.asciidoc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index c77040aa..2ff46315 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -120,7 +120,8 @@ And now let's see if our errors repro against Docker: ---- $ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] [...] -selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] +selenium.common.exceptions.NoSuchElementException: Message: Unable to locate +element: #id_logout; [...] [...] AssertionError: 'Check your email' not found in 'Server Error (500)' [...] @@ -275,6 +276,7 @@ command: First, set your email password in your terminal if you need to: +[role="skipme"] [subs="specialcharacters,quotes"] ---- $ *echo $EMAIL_PASSWORD* @@ -293,12 +295,12 @@ $ *docker build -t superlists . && docker run \ --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ -e DJANGO_SECRET_KEY=sekrit \ -e DJANGO_ALLOWED_HOST=localhost \ - -e EMAIL_PASSWORD \ <1> + -e EMAIL_PASSWORD \ -it superlists* ---- -<1> If you use `-e` without an `=something` argument, it sets the env var inside Docker - to the same value set in the current shell. +TIP: If you use `-e` without an `=something` argument, + it sets the env var inside Docker to the same value set in the current shell. It's like saying `-e EMAIL_PASSWORD=$EMAIL_PASSWORD` @@ -1396,6 +1398,7 @@ Perhaps a little ascii-art diagram will help: | .create_session_on_server | +-----+ +-------------+ | (on server) | | (locally) | +------------------------------+ +----------------------------+ +---- From 087a335b90477f0e856ae06fd32b4f9689655198 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 22 Jan 2025 16:59:47 +0000 Subject: [PATCH 40/63] Split out fiddly content to an appendix --- ...dix_fts_for_external_dependencies.asdiidoc | 589 ++++++++++++++++++ atlas.json | 1 + book.asciidoc | 1 + chapter_23_debugging_prod.asciidoc | 554 +--------------- source/chapter_23_debugging_prod/superlists | 2 +- 5 files changed, 599 insertions(+), 548 deletions(-) create mode 100644 appendix_fts_for_external_dependencies.asdiidoc diff --git a/appendix_fts_for_external_dependencies.asdiidoc b/appendix_fts_for_external_dependencies.asdiidoc new file mode 100644 index 00000000..67bea7f1 --- /dev/null +++ b/appendix_fts_for_external_dependencies.asdiidoc @@ -0,0 +1,589 @@ +[[appendix_fts_for_external_dependencies]] +[appendix] +== The Subtleties of Functionally Testing External Dependencies + +You might remember from <> +a point at which we wanted to test sending email from the server. + +Here were the options we considered: + +1. We could build a "real" end-to-end test, and have our tests + log in to an email server, and retrieve the email from there. + That's what I did in the first and second edition. + +2. You can use a service like Mailinator or Mailsac, + which give you an email account to send to, + and some APIs for checking what mail has been delivered. + +3. We can use an alternative, fake email backend, + whereby Django will save the emails to a file on disk for example, + and we can inspect them there. + +4. Or we could give up on testing email on the server. + If we have a minimal smoke test that the server _can_ send emails, + then we don't need to test that they are _actually_ delivered. + +In the end we decided not to bother, +but let's spend a bit of time in this appendix trying out options 1 and 3, +just to see some of the fiddliness and trade-offs involved. + + +=== How to Test Email End-To-End with POP3 + +Here's an example helper function that can retrieve a real email +from a real POP3 email server, +using the horrifically tortuous Python standard library POP3 client. + +To make it work, we'll need an email address to receive the email. +I signed up for a Yahoo account for testing, +but you can use any email service you like, as long as it offers POP3 access. + +You will need to set the +`RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. + +[subs="specialcharacters,quotes"] +---- +$ *export RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere* +---- + +[role="sourcecode skipme"] +.src/functional_tests/test_login.py (ch23l001) +==== +[source,python] +---- +import os +import poplib +import re +impot time +[...] + +def retrieve_pop3_email(receiver_email, subject, pop3_server, pop3_password): + email_id = None + start = time.time() + inbox = poplib.POP3_SSL(pop3_server) + try: + inbox.user(receiver_email) + inbox.pass_(pop3_password) + while time.time() - start < POP3_TIMEOUT: + # get 10 newest messages + count, _ = inbox.stat() + for i in reversed(range(max(1, count - 10), count + 1)): + print("getting msg", i) + _, lines, __ = inbox.retr(i) + lines = [l.decode("utf8") for l in lines] + print(lines) + if f"Subject: {subject}" in lines: + email_id = i + body = "\n".join(lines) + return body + time.sleep(5) + finally: + if email_id: + inbox.dele(email_id) + inbox.quit() +---- +==== + +If you're curious, I'd encourage you to try this out in your FTs. +It definitely _can_ work. +But, having tried it in the first couple of editions of the book. +I have to say it's fiddly to get right, +and often flaky, which is a highly undesirable property for a testing tool. +So let's leave that there for now. + + +=== Using a Fake Email Backend For Django + +Next let's investigate using a filesystem-based email backend. +As we'll see, although it definitely has the advantage +that everything stays local on our own machine +(there are no calls over the internet), +there are quite a few things to watch out for. + +Let's say that, if we detect an environment variable `EMAIL_FILE_PATH`, +we switch to Django's file-based backend: + + +.src/superlists/settings.py (ch23l002) +==== +[source,python] +---- +EMAIL_HOST = "smtp.gmail.com" +EMAIL_HOST_USER = "obeythetestinggoat@gmail.com" +EMAIL_HOST_PASSWORD = os.environ.get("EMAIL_PASSWORD") +EMAIL_PORT = 587 +EMAIL_USE_TLS = True +# Use fake file-based backend if EMAIL_FILE_PATH is set +if "EMAIL_FILE_PATH" in os.environ: + EMAIL_BACKEND = "django.core.mail.backends.filebased.EmailBackend" + EMAIL_FILE_PATH = os.environ["EMAIL_FILE_PATH"] +---- +==== + +Here's how we can adapt our tests to conditionally use the email file, +instead of Django's `mail.outbox`, if the env var is set when running our tests: + + + +[role="sourcecode"] +.src/functional_tests/test_login.py (ch23l003) +==== +[source,python] +---- +class LoginTest(FunctionalTest): + def retrieve_email_from_file(self, sent_to, subject, emails_dir): # <1> + latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] # <2> + latest_email = latest_emails_file.read_text().split("-" * 80)[-1] # <3> + self.assertIn(subject, latest_email) + self.assertIn(sent_to, latest_email) + return latest_email + + def retrieve_email_from_django_outbox(self, sent_to, subject): # <4> + email = mail.outbox.pop() + self.assertIn(sent_to, email.to) + self.assertEqual(email.subject, subject) + return email.body + + def wait_for_email(self, sent_to, subject): # <5> + """ + Retrieve email body, + from a file if the right env var is set, + or get it from django.mail.outbox by default + """ + if email_file_path := os.environ.get("EMAIL_FILE_PATH"): # <6> + return self.wait_for( # <7> + lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) + ) + else: + return self.retrieve_email_from_django_outbox(sent_to, subject) + + def test_login_using_magic_link(self): + [...] +---- +==== + +<1> Here's our helper method for getting email contents from a file. + It takes the configured email directory as an argument, + as well as the sent-to address and expected subject. + +<2> Django saves a new file with emails every time you restart the server. + The filename has a timestamp in it, + so we can get the latest one by sorting the files in our test directory. + Check out the https://docs.python.org/3/library/pathlib.html[Pathlib] docs + if you haven't used it before, it's a nice, relatively new way of working with files in Python. + +<3> The emails in the file are separated by a line of 80 hyphens. + +<4> This is the matching helper for getting the email from `mail.outbox`. + +<5> Here's where we dispatch to the right helper based on whether the env + var is set. + +<6> Checking whether an environment variable is set, and using its value if so, + is one of the (relatively few) places where it's nice to use the walrus operator. + +<7> I'm using a `wait_for()` here because anything involving reading and writing from files, + especially across the filesystem mounts inside and outside of Docker, + has a potential race condition. + + +We'll need a couple more minor changes to the FT, to use the helper: + + +[role="sourcecode"] +.src/functional_tests/test_login.py (ch23l004) +==== +[source,diff] +---- +@@ -59,15 +59,12 @@ class LoginTest(FunctionalTest): + ) + + # She checks her email and finds a message +- email = mail.outbox.pop() +- self.assertIn(TEST_EMAIL, email.to) +- self.assertEqual(email.subject, SUBJECT) ++ email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) + + # It has a URL link in it +- self.assertIn("Use this link to log in", email.body) +- url_search = re.search(r"http://.+/.+$", email.body) +- if not url_search: +- self.fail(f"Could not find url in email body:\n{email.body}") ++ self.assertIn("Use this link to log in", email_body) ++ if not (url_search := re.search(r"http://.+/.+$", email_body, re.MULTILINE)): ++ self.fail(f"Could not find url in email body:\n{email_body}") + url = url_search.group(0) + self.assertIn(self.live_server_url, url) +---- +==== + +// TODO backport that walrus + +Now let's set that file path, and mount it inside our docker container, +so that it's available both inside and outside the container: + +[subs="attributes+,specialcharacters,quotes"] +---- +# set a local env var for our path to the emails file +$ *export EMAIL_FILE_PATH=/tmp/superlists-emails* +# make sure the file exists +$ *mkdir -p $EMAIL_FILE_PATH* +# re-run our container, with the EMAIL_FILE_PATH as an env var, and mounted. +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ <1> + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ <2> + -it superlists* +---- + +<1> Here's where we mount the emails file so we can see it + both inside and outside the container + +<2> And here's where we pass the path as an env var, + once again re-exporting the variable from the current shell. + + +And we can re-run our FT, first without using Docker or the EMAIL_FILE_PATH, +just to check we didn't break anything: + + +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*./src/manage.py test functional_tests.test_login*] +[...] +OK +---- + +And now _with_ Docker and the EMAIL_FILE_PATH: + +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ + python src/manage.py test functional_tests* +[...] +OK +---- + + +It works! Hooray. + + +=== Double-Checking our Test and Our Fix + +As always, we should be suspicious of any test that we've only ever seen pass! +Let's see if we can make this test fail. + +Before we do--we've been in the detail for a bit, +it's worth reminding ourselves of what the actual bug was, +and how we're fixing it! +The bug was, the server was crashing when it tried to send an email. +The reason was, we hadn't set the `EMAIL_PASSWORD` environment variable. +We managed to repro the bug in Docker. +The actual _fix_ is to set that env var, +both in Docker and eventually on the server. +Now we want to have a _test_ that our fix works, +and we looked in to a few different options, +settling on using the `filebased.EmailBackend" +`EMAIL_BACKEND` setting using the `EMAIL_FILE_PATH` environment variable. + +Now, I say we haven't seen the test fail, +but actually we have, when we repro'd the bug. +If we unset the `EMAIL_PASSWORD` env var, it will fail again. +I'm more worried about the new parts of our tests, +the bits where we go and read from the file at `EMAIL_FILE_PATH`. +How can we make that part fail? + +Well, how about if we deliberately break our email-sending code? + + +[role="sourcecode"] +.src/accounts/views.py (ch23l005) +==== +[source,python] +---- +def send_login_email(request): + email = request.POST["email"] + token = Token.objects.create(email=email) + url = request.build_absolute_uri( + reverse("login") + "?token=" + str(token.uid), + ) + message_body = f"Use this link to log in:\n\n{url}" + # send_mail( <1> + # "Your login link for Superlists", + # message_body, + # "noreply@superlists", + # [email], + # ) + messages.success( + request, + "Check your email, we've sent you a link you can use to log in.", + ) + return redirect("/") +---- +==== + +<1> We just comment out the entire send_email block. + + +We rebuild our docker image: + + +[subs="specialcharacters,quotes"] +---- +# check our env var is set +$ *echo $EMAIL_FILE_PATH* +/tmp/superlists-emails +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ + -it superlists* +---- + +// TODO: aside on moujnting /src/? + +And we re-run our test: + + +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ + ./src/manage.py test functional_tests.test_login +[...] +Ran 1 test in 2.513s + +OK +---- + + +Eh? How did that pass? + + +=== Testing side-effects is fiddly! + +We've run into an example of the kinds of problems you often encounter +when our tests involve side-effects. + +Let's have a look in our test emails directory: + +[role="skipme"] +[subs="specialcharacters,quotes"] +---- +$ *ls $EMAIL_FILE_PATH* +20241120-153150-262004991022080.log +20241120-153154-262004990980688.log +20241120-153301-272143941669888.log +---- + +Every time we restart the server, it opens a new file, +but only when it first tries to send an email. +Because we've commented out the whole email-sending block, +our test instead picks up on an old email, +which still has a valid url in it, +because the token is still in the database. + +NOTE: You'll run into a similar issue if you test with "real" emails in POP3. + How do you make sure you're not picking up an email from a previous test run? + +Let's clear out the db: + +[subs="specialcharacters,quotes"] +---- +$ *rm src/db.sqlite3 && ./src/manage.py migrate* +Operations to perform: + Apply all migrations: accounts, auth, contenttypes, lists, sessions +Running migrations: + Applying accounts.0001_initial... OK + Applying accounts.0002_token... OK + Applying contenttypes.0001_initial... OK + Applying contenttypes.0002_remove_content_type_name... OK + Applying auth.0001_initial... OK +---- + + +And... + +cmdgg +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* +[...] +ERROR: test_login_using_magic_link (functional_tests.test_login.LoginTest.test_login_using_magic_link) + self.wait_to_be_logged_in(email=TEST_EMAIL) + ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^ +[...] +selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] +---- + +OK sure enough, the `wait_to_be_logged_in()` helper is failing, +because now, although we have found an email, its token is invalid. + + +Here's another way to make the tests fail: + +[subs="specialcharacters,macros"] +---- +$ pass:[rm $EMAIL_FILE_PATH/*] +---- + +Now when we run the FT: + +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* +ERROR: test_login_using_magic_link +(functional_tests.test_login.LoginTest.test_login_using_magic_link) +[...] + email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) +[...] + return self.wait_for( + ~~~~~~~~~~~~~^ + lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +[...] + latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^ +IndexError: list index out of range +---- + +We see there are no email files, because we're not sending one. + +NOTE: In this configuration of Docker + `filebase.EmailBackend`, + we now have to manage side effects in two locations: + the database at _src/db.sqlite3_, and the email files in _/tmp_. + What Django used to do for us thanks to LiveServerTestCase + is now all our responsibility, and as you can see, it's hard to get right. + This is a tradeoff to be aware of when writing tests against "real" systems. + + +Still, this isn't quite satisfactory. +Let's try a different way to make our tests fail, +where we _will_ send an email, but we'll give it the wrong contents: + + +[role="sourcecode"] +.src/accounts/views.py (ch23l006) +==== +[source,python] +---- +def send_login_email(request): + email = request.POST["email"] + token = Token.objects.create(email=email) + url = request.build_absolute_uri( + reverse("login") + "?token=" + str(token.uid), + ) + message_body = f"Use this link to log in:\n\n{url}" + send_mail( + "Your login link for Superlists", + "HAHA NO LOGIN URL FOR U", # <1> + "noreply@superlists", + [email], + ) + messages.success( + request, + "Check your email, we've sent you a link you can use to log in.", + ) + return redirect("/") +---- +==== + +<1> We _do_ send an email, but it won't contain a login URL. + +Let's rebuild again: + +[subs="specialcharacters,quotes"] +---- +# check our env var is set +$ *echo $EMAIL_FILE_PATH* +/tmp/superlists-emails +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -e EMAIL_FILE_PATH \ + -it superlists* +---- + +Now how do our tests look? + +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] +FAIL: test_login_using_magic_link +(functional_tests.test_login.LoginTest.test_login_using_magic_link) +[...] + email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) +[...] + self.assertIn("Use this link to log in", email_body) + ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +AssertionError: 'Use this link to log in' not found in 'Content-Type: +text/plain; charset="utf-8"\nMIME-Version: 1.0\nContent-Transfer-Encoding: +7bit\nSubject: Your login link for Superlists\nFrom: noreply@superlists\nTo: +edith@example.com\nDate: Wed, 13 Nov 2024 18:00:55 -0000\nMessage-ID: +[...]\n\nHAHA NO LOGIN URL FOR +U\n-------------------------------------------------------------------------------\n' +---- + +OK good, that's the error we wanted! +I think we can be fairly confident that this testing setup +can genuinely test that emails are sent properly. +Let's revert our temporarily-broken _views.py_, +rebuild, and make sure the tests pass once again. + +[subs="specialcharacters,quotes"] +---- +$ *git stash* +$ *docker build [...]* +# separate terminal +$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails [...] +[...] +OK +---- + + +NOTE: It may seem like I've gone through a lot of back-and-forth, + but I wanted to give you a flavour of the fiddliness involved + in these kinds of tests that involve a lot of side-effects. + + +=== Decision Time: Which Test Strategy Will We Keep + +Let's recap our three options: + + +.Testing Strategy Tradeoffs +[cols="1,1,1"] +|======= +| Strategy | Pros | Cons +| End-to-end with POP3 | Maximally realistic, tests the whole system | Slow, fiddly, unreliable +| File-based fake email backend | Faster, more reliable, no network calls, tests end-to-end (albeit with fake components) | Still Fiddly, requires managing db & filesystem side-effects +| Give up on testing email on the server/Docker | Fast, simple | Less confidence that things work "for real" +|======= + +This is a common problem in testing integration with external systems, +how far should we go? How realistic should we make our tests? + +In the book in the end, I suggested we go for the last option, +ie give up. Email itself is a well-understood protocol +(reader, it's been around since _before I was born_, and that's a whiles ago now) +and Django has supported sending email for more than a decade, +so I think we can afford to say, in this case, +that the costs of building testing tools for email outweigh the benefits. + +But not all external dependencies are as well-understood as email. +If you're working with a new API, or a new service, +you may well decide it's worth putting in the effort to get a "real" end-to-end functional test to work. + + +* TODO: recap diff --git a/atlas.json b/atlas.json index 42f7566e..a60c076e 100644 --- a/atlas.json +++ b/atlas.json @@ -44,6 +44,7 @@ "appendix_I_PythonAnywhere.asciidoc", "appendix_Django_Class-Based_Views.asciidoc", "appendix_IV_testing_migrations.asciidoc", + "appendix_fts_for_external_dependencies.asciidoc", "appendix_purist_unit_tests.asciidoc", "appendix_bdd.asciidoc", "appendix_rest_api.asciidoc", diff --git a/book.asciidoc b/book.asciidoc index a897f69b..1efbe0ed 100644 --- a/book.asciidoc +++ b/book.asciidoc @@ -57,6 +57,7 @@ include::epilogue.asciidoc[] include::appendix_I_PythonAnywhere.asciidoc[] include::appendix_Django_Class-Based_Views.asciidoc[] include::appendix_IV_testing_migrations.asciidoc[] +include::appendix_fts_for_external_dependencies.asciidoc[] include::appendix_purist_unit_tests.asciidoc[] include::appendix_bdd.asciidoc[] include::appendix_rest_api.asciidoc[] diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 2ff46315..6a161ef1 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -338,6 +338,7 @@ We need another technique if we want to actually inspect the emails that the server sends, in our tests against Docker or staging. +[[options-for-testing-real-email]] === Deciding How to Test "Real" Email Sending This is a point at which we have to explore some tradeoffs. @@ -360,552 +361,7 @@ There are a few different ways we could test this: then we don't need to test that they are _actually_ delivered. -I'm not going to explore option 2 in this book, -since it involves a commercial service and I don't want to endorse one, -but that's not to say it's a bad option. -Especially since they have free plans these days! - -But let's take a quick look at options 1 and 3 the and their pros+cons. - - -=== How to Test Email End-To-End with POP3 - -Here's an example helper function that can retrieve a real email -from a real POP3 email server, -using the horrifically tortuous Python standard library POP3 client. - -To make it work, we'll need an email address to receive the email. -I signed up for a Yahoo account for testing, -but you can use any email service you like, as long as it offers POP3 access. - -You will need to set the -`RECEIVER_EMAIL_PASSWORD` environment variable in the console that's running the FT. - -[subs="specialcharacters,quotes"] ----- -$ *export RECEIVER_EMAIL_PASSWORD=otheremailpasswordhere* ----- - -[role="sourcecode skipme"] -.src/functional_tests/test_login.py (ch23l001) -==== -[source,python] ----- -import os -import poplib -import re -impot time -[...] - -def retrieve_pop3_email(receiver_email, subject, pop3_server, pop3_password): - email_id = None - start = time.time() - inbox = poplib.POP3_SSL(pop3_server) - try: - inbox.user(receiver_email) - inbox.pass_(pop3_password) - while time.time() - start < POP3_TIMEOUT: - # get 10 newest messages - count, _ = inbox.stat() - for i in reversed(range(max(1, count - 10), count + 1)): - print("getting msg", i) - _, lines, __ = inbox.retr(i) - lines = [l.decode("utf8") for l in lines] - print(lines) - if f"Subject: {subject}" in lines: - email_id = i - body = "\n".join(lines) - return body - time.sleep(5) - finally: - if email_id: - inbox.dele(email_id) - inbox.quit() ----- -==== - -If you're curious, I'd encourage you to try this out in your FTs. -It definitely _can_ work. -But, having tried it in the first couple of editions of the book. -I have to say it's fiddly to get right, -and often flaky, which is a highly undesirable property for a testing tool. -So let's leave that there for now. - -TIP: If you _do_ want to test email end-to-end, - I'd encourage you to investigate services like Mailinator or Mailsac, - rather than trying to use POP3 directly. - - - -=== Using a Fake Email Backend For Django - -Next let's investigate using a filesystem-based email backend. -As we'll see, although it definitely has the advantage -that everything stays local on our own machine -(there are no calls over the internet), -there are quite a few things to watch out for. - -Let's say that, if we detect an environment variable `EMAIL_FILE_PATH`, -we switch to Django's file-based backend: - - -.src/superlists/settings.py (ch23l002) -==== -[source,python] ----- -EMAIL_HOST = "smtp.gmail.com" -EMAIL_HOST_USER = "obeythetestinggoat@gmail.com" -EMAIL_HOST_PASSWORD = os.environ.get("EMAIL_PASSWORD") -EMAIL_PORT = 587 -EMAIL_USE_TLS = True -# Use fake file-based backend if EMAIL_FILE_PATH is set -if "EMAIL_FILE_PATH" in os.environ: - EMAIL_BACKEND = "django.core.mail.backends.filebased.EmailBackend" - EMAIL_FILE_PATH = os.environ["EMAIL_FILE_PATH"] ----- -==== - -Here's how we can adapt our tests to conditionally use the email file, -instead of Django's `mail.outbox`, if the env var is set when running our tests: - - - -[role="sourcecode"] -.src/functional_tests/test_login.py (ch23l003) -==== -[source,python] ----- -class LoginTest(FunctionalTest): - def retrieve_email_from_file(self, sent_to, subject, emails_dir): # <1> - latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] # <2> - latest_email = latest_emails_file.read_text().split("-" * 80)[-1] # <3> - self.assertIn(subject, latest_email) - self.assertIn(sent_to, latest_email) - return latest_email - - def retrieve_email_from_django_outbox(self, sent_to, subject): # <4> - email = mail.outbox.pop() - self.assertIn(sent_to, email.to) - self.assertEqual(email.subject, subject) - return email.body - - def wait_for_email(self, sent_to, subject): # <5> - """ - Retrieve email body, - from a file if the right env var is set, - or get it from django.mail.outbox by default - """ - if email_file_path := os.environ.get("EMAIL_FILE_PATH"): # <6> - return self.wait_for( # <7> - lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) - ) - else: - return self.retrieve_email_from_django_outbox(sent_to, subject) - - def test_login_using_magic_link(self): - [...] ----- -==== - -<1> Here's our helper method for getting email contents from a file. - It takes the configured email directory as an argument, - as well as the sent-to address and expected subject. - -<2> Django saves a new file with emails every time you restart the server. - The filename has a timestamp in it, - so we can get the latest one by sorting the files in our test directory. - Check out the https://docs.python.org/3/library/pathlib.html[Pathlib] docs - if you haven't used it before, it's a nice, relatively new way of working with files in Python. - -<3> The emails in the file are separated by a line of 80 hyphens. - -<4> This is the matching helper for getting the email from `mail.outbox`. - -<5> Here's where we dispatch to the right helper based on whether the env - var is set. - -<6> Checking whether an environment variable is set, and using its value if so, - is one of the (relatively few) places where it's nice to use the walrus operator. - -<7> I'm using a `wait_for()` here because anything involving reading and writing from files, - especially across the filesystem mounts inside and outside of Docker, - has a potential race condition. - - -We'll need a couple more minor changes to the FT, to use the helper: - - -[role="sourcecode"] -.src/functional_tests/test_login.py (ch23l004) -==== -[source,diff] ----- -@@ -59,15 +59,12 @@ class LoginTest(FunctionalTest): - ) - - # She checks her email and finds a message -- email = mail.outbox.pop() -- self.assertIn(TEST_EMAIL, email.to) -- self.assertEqual(email.subject, SUBJECT) -+ email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) - - # It has a URL link in it -- self.assertIn("Use this link to log in", email.body) -- url_search = re.search(r"http://.+/.+$", email.body) -- if not url_search: -- self.fail(f"Could not find url in email body:\n{email.body}") -+ self.assertIn("Use this link to log in", email_body) -+ if not (url_search := re.search(r"http://.+/.+$", email_body, re.MULTILINE)): -+ self.fail(f"Could not find url in email body:\n{email_body}") - url = url_search.group(0) - self.assertIn(self.live_server_url, url) ----- -==== - -// TODO backport that walrus - -Now let's set that file path, and mount it inside our docker container, -so that it's available both inside and outside the container: - -[subs="attributes+,specialcharacters,quotes"] ----- -# set a local env var for our path to the emails file -$ *export EMAIL_FILE_PATH=/tmp/superlists-emails* -# make sure the file exists -$ *mkdir -p $EMAIL_FILE_PATH* -# re-run our container, with the EMAIL_FILE_PATH as an env var, and mounted. -$ *docker build -t superlists . && docker run \ - -p 8888:8888 \ - --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ - --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ <1> - -e DJANGO_SECRET_KEY=sekrit \ - -e DJANGO_ALLOWED_HOST=localhost \ - -e EMAIL_PASSWORD \ - -e EMAIL_FILE_PATH \ <2> - -it superlists* ----- - -<1> Here's where we mount the emails file so we can see it - both inside and outside the container - -<2> And here's where we pass the path as an env var, - once again re-exporting the variable from the current shell. - - -And we can re-run our FT, first without using Docker or the EMAIL_FILE_PATH, -just to check we didn't break anything: - - -[subs="specialcharacters,macros"] ----- -$ pass:quotes[*./src/manage.py test functional_tests.test_login*] -[...] -OK ----- - -And now _with_ Docker and the EMAIL_FILE_PATH: - -[subs="specialcharacters,quotes"] ----- -$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ - python src/manage.py test functional_tests* -[...] -OK ----- - - -It works! Hooray. - - -=== Double-Checking our Test and Our Fix - -As always, we should be suspicious of any test that we've only ever seen pass! -Let's see if we can make this test fail. - -Before we do--we've been in the detail for a bit, -it's worth reminding ourselves of what the actual bug was, -and how we're fixing it! -The bug was, the server was crashing when it tried to send an email. -The reason was, we hadn't set the `EMAIL_PASSWORD` environment variable. -We managed to repro the bug in Docker. -The actual _fix_ is to set that env var, -both in Docker and eventually on the server. -Now we want to have a _test_ that our fix works, -and we looked in to a few different options, -settling on using the `filebased.EmailBackend" -`EMAIL_BACKEND` setting using the `EMAIL_FILE_PATH` environment variable. - -Now, I say we haven't seen the test fail, -but actually we have, when we repro'd the bug. -If we unset the `EMAIL_PASSWORD` env var, it will fail again. -I'm more worried about the new parts of our tests, -the bits where we go and read from the file at `EMAIL_FILE_PATH`. -How can we make that part fail? - -Well, how about if we deliberately break our email-sending code? - - -[role="sourcecode"] -.src/accounts/views.py (ch23l005) -==== -[source,python] ----- -def send_login_email(request): - email = request.POST["email"] - token = Token.objects.create(email=email) - url = request.build_absolute_uri( - reverse("login") + "?token=" + str(token.uid), - ) - message_body = f"Use this link to log in:\n\n{url}" - # send_mail( <1> - # "Your login link for Superlists", - # message_body, - # "noreply@superlists", - # [email], - # ) - messages.success( - request, - "Check your email, we've sent you a link you can use to log in.", - ) - return redirect("/") ----- -==== - -<1> We just comment out the entire send_email block. - - -We rebuild our docker image: - - -[subs="specialcharacters,quotes"] ----- -# check our env var is set -$ *echo $EMAIL_FILE_PATH* -/tmp/superlists-emails -$ *docker build -t superlists . && docker run \ - -p 8888:8888 \ - --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ - --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ - -e DJANGO_SECRET_KEY=sekrit \ - -e DJANGO_ALLOWED_HOST=localhost \ - -e EMAIL_PASSWORD \ - -e EMAIL_FILE_PATH \ - -it superlists* ----- - -// TODO: aside on moujnting /src/? - -And we re-run our test: - - -[subs="specialcharacters,quotes"] ----- -$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails \ - ./src/manage.py test functional_tests.test_login -[...] -Ran 1 test in 2.513s - -OK ----- - - -Eh? How did that pass? - - -=== Testing side-effects is fiddly! - -We've run into an example of the kinds of problems you often encounter -when our tests involve side-effects. - -Let's have a look in our test emails directory: - -[role="skipme"] -[subs="specialcharacters,quotes"] ----- -$ *ls $EMAIL_FILE_PATH* -20241120-153150-262004991022080.log -20241120-153154-262004990980688.log -20241120-153301-272143941669888.log ----- - -Every time we restart the server, it opens a new file, -but only when it first tries to send an email. -Because we've commented out the whole email-sending block, -our test instead picks up on an old email, -which still has a valid url in it, -because the token is still in the database. - - -Let's clear out the db: - -[subs="specialcharacters,quotes"] ----- -$ *rm src/db.sqlite3 && ./src/manage.py migrate* -Operations to perform: - Apply all migrations: accounts, auth, contenttypes, lists, sessions -Running migrations: - Applying accounts.0001_initial... OK - Applying accounts.0002_token... OK - Applying contenttypes.0001_initial... OK - Applying contenttypes.0002_remove_content_type_name... OK - Applying auth.0001_initial... OK ----- - - -And... - -cmdgg -[subs="specialcharacters,quotes"] ----- -$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* -[...] -ERROR: test_login_using_magic_link (functional_tests.test_login.LoginTest.test_login_using_magic_link) - self.wait_to_be_logged_in(email=TEST_EMAIL) - ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^ -[...] -selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: #id_logout; [...] ----- - -OK sure enough, the `wait_to_be_logged_in()` helper is failing, -because now, although we have found an email, its token is invalid. - - -Here's another way to make the tests fail: - -[subs="specialcharacters,macros"] ----- -$ pass:[rm $EMAIL_FILE_PATH/*] ----- - -Now when we run the FT: - -[subs="specialcharacters,quotes"] ----- -$ *TEST_SERVER=localhost:8888 ./src/manage.py test functional_tests.test_login* -ERROR: test_login_using_magic_link -(functional_tests.test_login.LoginTest.test_login_using_magic_link) -[...] - email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) -[...] - return self.wait_for( - ~~~~~~~~~~~~~^ - lambda: self.retrieve_email_from_file(sent_to, subject, email_file_path) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -[...] - latest_emails_file = sorted(Path(emails_dir).iterdir())[-1] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^ -IndexError: list index out of range ----- - -We see there are no email files, because we're not sending one. - -NOTE: In this configuration of Docker + `filebase.EmailBackend`, - we now have to manage side effects in two locations: - the database at _src/db.sqlite3_, and the email files in _/tmp_. - What Django used to do for us thanks to LiveServerTestCase - is now all our responsibility, and as you can see, it's hard to get right. - This is a tradeoff to be aware of when writing tests against "real" systems. - - -Still, this isn't quite satisfactory. -Let's try a different way to make our tests fail, -where we _will_ send an email, but we'll give it the wrong contents: - - -[role="sourcecode"] -.src/accounts/views.py (ch23l006) -==== -[source,python] ----- -def send_login_email(request): - email = request.POST["email"] - token = Token.objects.create(email=email) - url = request.build_absolute_uri( - reverse("login") + "?token=" + str(token.uid), - ) - message_body = f"Use this link to log in:\n\n{url}" - send_mail( - "Your login link for Superlists", - "HAHA NO LOGIN URL FOR U", # <1> - "noreply@superlists", - [email], - ) - messages.success( - request, - "Check your email, we've sent you a link you can use to log in.", - ) - return redirect("/") ----- -==== - -<1> We _do_ send an email, but it won't contain a login URL. - -Let's rebuild again: - -[subs="specialcharacters,quotes"] ----- -# check our env var is set -$ *echo $EMAIL_FILE_PATH* -/tmp/superlists-emails -$ *docker build -t superlists . && docker run \ - -p 8888:8888 \ - --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ - --mount type=bind,source=$EMAIL_FILE_PATH,target=$EMAIL_FILE_PATH \ - -e DJANGO_SECRET_KEY=sekrit \ - -e DJANGO_ALLOWED_HOST=localhost \ - -e EMAIL_PASSWORD \ - -e EMAIL_FILE_PATH \ - -it superlists* ----- - -Now how do our tests look? - -[subs="specialcharacters,macros"] ----- -$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests*] -FAIL: test_login_using_magic_link -(functional_tests.test_login.LoginTest.test_login_using_magic_link) -[...] - email_body = self.wait_for_email(TEST_EMAIL, SUBJECT) -[...] - self.assertIn("Use this link to log in", email_body) - ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -AssertionError: 'Use this link to log in' not found in 'Content-Type: -text/plain; charset="utf-8"\nMIME-Version: 1.0\nContent-Transfer-Encoding: -7bit\nSubject: Your login link for Superlists\nFrom: noreply@superlists\nTo: -edith@example.com\nDate: Wed, 13 Nov 2024 18:00:55 -0000\nMessage-ID: -[...]\n\nHAHA NO LOGIN URL FOR -U\n-------------------------------------------------------------------------------\n' ----- - -OK good, that's the error we wanted! -I think we can be fairly confident that this testing setup -can genuinely test that emails are sent properly. -Let's revert our temporarily-broken _views.py_, -rebuild, and make sure the tests pass once again. - -[subs="specialcharacters,quotes"] ----- -$ *git stash* -$ *docker build [...]* -# separate terminal -$ *TEST_SERVER=localhost:8888 EMAIL_FILE_PATH=/tmp/superlists-emails [...] -[...] -OK ----- - -// todo: aside or title here? - -NOTE: It may seem like we've done a lot of back-and-forth, - and I could have written the book without this little detour to make the tests fail, - or I could have skipped one of the weird bugs at least, - but I wanted to give you a flavour of the fiddliness involved - in these kinds of tests that involve a lot of side-effects. - - -=== Decision Time: Which Test Strategy Will We Keep - -Let's recap our three options: +But let's lay out some of the pros + cons: .Testing Strategy Tradeoffs @@ -913,12 +369,13 @@ Let's recap our three options: |======= | Strategy | Pros | Cons | End-to-end with POP3 | Maximally realistic, tests the whole system | Slow, fiddly, unreliable +| Email testing service eg Mailinator/Mailsac| As realistic as real POP3, with better APIs for testing| Slow, possibly expensive. Plus I don't want to endorse any particular commercial provider ;-) | File-based fake email backend | Faster, more reliable, no network calls, tests end-to-end (albeit with fake components) | Still Fiddly, requires managing db & filesystem side-effects | Give up on testing email on the server/Docker | Fast, simple | Less confidence that things work "for real" |======= This is a common problem in testing integration with external systems, -how far should we go? How realistic should we make our tests. +how far should we go? How realistic should we make our tests? In this case, I'm going to suggest we go for the last option, which is not to test email sending on the server or in Docker. @@ -935,6 +392,9 @@ and we configure our FTs to just check that the server (or Docker) _seems_ to be (in the sense of "not crashing") and we can skip the bit where we check the email contents in our FT. Remember, we also have unit tests for the email content! +NOTE: I explore some of the difficulties involved in getting + these kinds of tests to work in <>, + so go check that out if this feels like a bit of a cop-out! Here's where we can put an early return in the FT: diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 1b0004f4..4abb5e7d 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 1b0004f4943d56d401551c1ff941b9cb025e78ab +Subproject commit 4abb5e7da5833a549b8d29385e7ee0659c807b6b From 3fc819b47ae352eccfb4df1fbf2f71a7786ddfa4 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:12:23 +0000 Subject: [PATCH 41/63] fix a couple of .yaml.yamls --- chapter_11_server_prep.asciidoc | 4 ++-- chapter_18_second_deploy.asciidoc | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/chapter_11_server_prep.asciidoc b/chapter_11_server_prep.asciidoc index dcf8a015..10e53e74 100644 --- a/chapter_11_server_prep.asciidoc +++ b/chapter_11_server_prep.asciidoc @@ -580,7 +580,7 @@ Here's the full command we'll use, with an explanation of each part: ansible-playbook \ --user=elspeth \ <1> -i staging.ottg.co.uk, \ <2><3> - infra/deploy-playbook.yaml.yaml \ <4> + infra/deploy-playbook.yaml \ <4> -vv <5> ---- @@ -610,7 +610,7 @@ Here's some example output when I run it: [subs="specialcharacters,macros"] ---- -$ pass:quotes[*ansible-playbook --user=elspeth -i staging.ottg.co.uk, infra/deploy-playbook.yaml.yaml -vv*] +$ pass:quotes[*ansible-playbook --user=elspeth -i staging.ottg.co.uk, infra/deploy-playbook.yaml -vv*] ansible-playbook [core 2.17.5] config file = None configured module search path = ['~/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules'] diff --git a/chapter_18_second_deploy.asciidoc b/chapter_18_second_deploy.asciidoc index 1fe3963d..a86bb15a 100644 --- a/chapter_18_second_deploy.asciidoc +++ b/chapter_18_second_deploy.asciidoc @@ -37,11 +37,11 @@ We start with the staging server: [role="against-server small-code"] [subs="specialcharacters,macros"] ---- -$ pass:quotes[*ansible-playbook --user=elspeth -i staging.ottg.co.uk, infra/deploy-playbook.yaml.yaml -vv*] +$ pass:quotes[*ansible-playbook --user=elspeth -i staging.ottg.co.uk, infra/deploy-playbook.yaml -vv*] [...] -PLAYBOOK: deploy-playbook.yaml.yaml *********************************************** -1 plays in infra/deploy-playbook.yaml.yaml +PLAYBOOK: deploy-playbook.yaml *********************************************** +1 plays in infra/deploy-playbook.yaml PLAY [all] ********************************************************************* @@ -118,18 +118,16 @@ OK Hooray! - [role="pagebreak-before less_space"] -=== Live Deploy +=== Production Deploy -// RITA: Forgive me if this is a newbie question, but will readers understand what you mean by "live"? As in, "live what"? Just making sure that it's clear. -Assuming all is well, we then run our deploy against live: +Since all is looking well we can deploy to prod! [role="against-server"] [subs="specialcharacters,macros"] ---- -$ pass:quotes[*ansible-playbook --user=elspeth -i www.ottg.co.uk, infra/deploy-playbook.yaml.yaml -vv*] +$ pass:quotes[*ansible-playbook --user=elspeth -i www.ottg.co.uk, infra/deploy-playbook.yaml -vv*] ---- From 864ced15cd525f113556d918d6d5f1769423ddd2 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:12:59 +0000 Subject: [PATCH 42/63] update submodule --- source/chapter_23_debugging_prod/superlists | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 4abb5e7d..1b0004f4 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 4abb5e7da5833a549b8d29385e7ee0659c807b6b +Subproject commit 1b0004f4943d56d401551c1ff941b9cb025e78ab From f2735203f4a6bdc3643093a63c64332df96c4ae7 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:16:52 +0000 Subject: [PATCH 43/63] Bring back prod deploy in ansible chapter, fix some links --- chapter_12_ansible.asciidoc | 41 +++++++++++++++---------------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/chapter_12_ansible.asciidoc b/chapter_12_ansible.asciidoc index 68d5ff15..7edb2397 100644 --- a/chapter_12_ansible.asciidoc +++ b/chapter_12_ansible.asciidoc @@ -1060,32 +1060,28 @@ Rewire the FT runner to be able to test against the local VM. Having a Vagrant config file is particularly helpful when working in a team--it helps new developers to spin up servers that look exactly like yours.((("", startref="ansible29"))) +//// -Deploying to Live -^^^^^^^^^^^^^^^^^ +=== Deploying to Prod -TODO update this So, let's try using it for our live site! [role="small-code against-server"] [subs=""] ---- -$ fab deploy:host=elspeth@superlists.ottg.co.uk +$ pass:quotes[*ansible-playbook --user=elspeth -i www.ottg.co.uk, infra/deploy-playbook.yaml -vv*] +[...] Done. -Disconnecting from elspeth@superlists.ottg.co.uk... done. +Disconnecting from elspeth@www.ottg.co.uk... done. ---- -'Brrp brrp brpp'. You can see the script follows a slightly different path, -doing a `git clone` to bring down a brand new repo instead of a `git pull`. -It also needs to set up a new virtualenv from scratch, including a fresh -install of pip and Django. The `collectstatic` actually creates new files this -time, and the `migrate` seems to have worked too. +_Brrp brrp brpp_. Looking good? Go take a click around your live site! @@ -1105,7 +1101,7 @@ $ *git tag LIVE* $ *export TAG=$(date +DEPLOYED-%F/%H%M)* # this generates a timestamp $ *echo $TAG* # should show "DEPLOYED-" and then the timestamp $ *git tag $TAG* -$ *git push origin LIVE $TAG* # pushes the tags up +$ *git push origin LIVE $TAG* # pushes the tags up to GitHub ---- Now it's easy, at any time, to check what the difference is @@ -1120,9 +1116,13 @@ $ *git log --graph --oneline --decorate* [...] ---- -//// +NOTE: Once again, this use of git tags isn't meant to be the One True Way. + We just need some sort of way of keeping track of what was deployed when. + + +=== Tell everyone! + -// RITA: Perhaps add the note about the reader emailing you when their site goes live to this point. "Tell your mum! Tell me! Email me at...."" You now have a live website! Tell all your friends! Tell your mum, if no one else is interested! Or, tell me! I'm always delighted to see a new reader's site! @@ -1133,7 +1133,7 @@ In the next chapter, it's back to coding again.((("", startref="Fstage11"))) // DAVID: maybe more of a conclusion here? It's quite a heavy chapter, // a bit of an anticlimax to stop here. I want some inspiring note to end on. // In particular, how does this tie into TDD? -// DAVID: Also - now it's on staging, should we release to prod too? + === Further Reading @@ -1145,20 +1145,13 @@ and lots, lots more to learn besides. Here are some resources I used for inspiration: -* https://12factor.net/[The 12-factor App] by the Heroku team +* The original https://12factor.net/[12-factor App] manifesto from the Heroku team -* http://hynek.me/talks/python-deployments[Solid Python Deployments for Everybody] by Hynek Schlawack -// CSANAD: the author suggests another, slightly more up-to date (from 2018) -// talk now: https://hynek.me/talks/deploy-friendly/ +* https://hynek.me/talks/deploy-friendly/[How to Write Deployment-Friendly Apps] by Hynek Schlawack * The deployment chapter of - https://www.feldroy.com/books/two-scoops-of-django-3-x[Two Scoops of Django] + https://www.feldroy.com/two-scoops-of-django[Two Scoops of Django] by Dan Greenfeld and Audrey Roy -// CSANAD: this is 404 now. The book no longer seems to have a separate page -// instead, they list all their books at -// https://www.feldroy.com/two-scoops-press - -] [role="pagebreak-before less_space"] From fa0d2a0819bc6ea02dbabc502cad5fd3dad72e49 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:18:22 +0000 Subject: [PATCH 44/63] Flesh out pre-deploy checklist in 18, precursor to 23 --- chapter_18_second_deploy.asciidoc | 97 +++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 6 deletions(-) diff --git a/chapter_18_second_deploy.asciidoc b/chapter_18_second_deploy.asciidoc index a86bb15a..72a5ad10 100644 --- a/chapter_18_second_deploy.asciidoc +++ b/chapter_18_second_deploy.asciidoc @@ -3,15 +3,16 @@ ((("deployment", "procedure for", id="Dpro17"))) It's time to deploy our brilliant new validation code to our live servers. + This will be a chance to see our automated deploy scripts in action for the second time. +Let's take the opportunity to make a little deployment checklist. -// RITA: A long section where? In the book? Please clarify. NOTE: At this point I always want to say a huge thanks to Andrew Godwin and the whole Django team. - Up until Django 1.7, I used to have a whole long section, + In the first edition, I used to have a whole long section, entirely devoted to migrations. - Migrations now "just work", so I was able to drop it altogether. + Since Django 1.7, migrations now "just work", so I was able to drop it altogether. I mean yes this all happened nearly ten years ago, but still--open source software is a gift. We get such amazing things, entirely for free. @@ -28,11 +29,93 @@ You can refer back to <> for reminders on Ansible comman ******************************************************************************* +=== The Deployment Checklist + +Let's make a little checklist of pre-deployment tasks: + +1. We run all our unit and functional tests in the regular way. Just in case! +2. We rebuild our Docker image, and run our tests against Docker, on our local machine. +3. We deploy to staging, and run our FTs against staging. +4. Now we can deploy to prod. + +TIP: A deployment checklist like this should be a temporary measure. + Once you've worked through it manually a few times, + you should be looking to take the next step in automation, + continuous deployment straight using a CI/CD pipeline. + We'll touch on this in <>. + + + +=== A Full Test Run Locally + +Of course, under the watchful eye of the Testing Goat, +we're running the tests all the time! But, just in case: + +[subs="specialcharacters,quotes"] +---- +$ *cd src && python manage.py test* +[...] + +Ran 39 tests in 15.222s + +OK +---- + + +=== Quick Test Run Against Docker + +The next step closer to prod, is running things in Docker. +This was one of the main reasons we went to the trouble of containerising our app, +which is being able to repro the production environment as faithfully as possible, +on our own machine. -=== Staging Deploy +So let's rebuild our Docker image and spin up a local Docker container: -We start with the staging server: +[subs="specialcharacters,quotes"] +---- +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -it superlists + => [internal] load build definition from Dockerfile 0.0s + => => transferring dockerfile: 371B 0.0s + => [internal] load metadata for docker.io/library/python:3.13-slim 1.4s + [...] + => => naming to docker.io/library/superlists 0.0s ++ docker run -p 8888:8888 --mount +type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 -e +DJANGO_SECRET_KEY=sekrit -e DJANGO_ALLOWED_HOST=localhost -e EMAIL_PASSWORD -it +superlists +[2025-01-27 21:29:37 +0000] [7] [INFO] Starting gunicorn 22.0.0 +[2025-01-27 21:29:37 +0000] [7] [INFO] Listening at: http://0.0.0.0:8888 (7) +[2025-01-27 21:29:37 +0000] [7] [INFO] Using worker: sync +[2025-01-27 21:29:37 +0000] [8] [INFO] Booting worker with pid: 8 +---- + +And now, in a separate terminal, we can run our FT suite against the Docker: + +[subs="specialcharacters,quotes"] +---- +$ *TEST_SERVER=localhost:8888 python src/manage.py test functional_tests* +[...] +...... + --------------------------------------------------------------------- +Ran 6 tests in 17.047s + +OK +---- + +Looking good! Let's move on to staging. + + + +=== Staging Deploy and Test Run + + +Here's our `ansible-playbook` command to deploy to staging: [role="against-server small-code"] [subs="specialcharacters,macros"] @@ -86,7 +169,7 @@ Disconnecting from staging.ottg.co.uk... done. ---- -And run the tests against staging: +And now we run the FTs against staging: [role="small-code"] [subs="specialcharacters,macros"] @@ -94,6 +177,7 @@ And run the tests against staging: $ pass:quotes[*TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests*] OK ---- + // CSANAD: I needed to add `force_source` to the "Import container image on // server" task. Otherwise, the server would deploy a container based on // the old image, even though a new one was successfully created locally (and @@ -150,6 +234,7 @@ At this point you have two choices: 2. Learn about data migrations. See <>. + ==== How to Delete the Database on the Staging Server Here's how you might do option (1): From 065e5012feaeabe519c8df335c787611d52ccc6a Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:18:38 +0000 Subject: [PATCH 45/63] syntax stuff in 21 --- chapter_21_mocking_2.asciidoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/chapter_21_mocking_2.asciidoc b/chapter_21_mocking_2.asciidoc index 49200f13..489243b3 100644 --- a/chapter_21_mocking_2.asciidoc +++ b/chapter_21_mocking_2.asciidoc @@ -1497,12 +1497,12 @@ urlpatterns = [ And that gets us a fully passing FT--indeed, a fully passing test suite: -[subs="specialcharacters,macros"] +[subs="specialcharacters,quotes"] ---- -$ pass:quotes[*python src/manage.py test functional_tests.test_login*] +$ *python src/manage.py test functional_tests.test_login* [...] OK -$ pass:[cd src && python manage.py test] +$ *cd src && python manage.py test* [...] Ran 57 tests in 78.124s From e8c16abe4b1e878ba20f700abadbc49b49324127 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 22:26:47 +0000 Subject: [PATCH 46/63] Do Docker first in 23 --- chapter_23_debugging_prod.asciidoc | 140 +++-------------------------- 1 file changed, 13 insertions(+), 127 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 6a161ef1..c7d80a81 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -18,88 +18,14 @@ Oh yes, waiting to be logged in. And why was that? Ah yes, we had just built a way of pre-authenticating a user. Let's see how that works against our staging server and Docker. -// TODO revise this? -In this chapter we'll encounter some situations where the right answer is not always obvious. -We're into the realm of tradeoffs now, which is appropriate for this more advanced part of the book. -My aim is no longer to show you a single, good way to do things, -but instead to give you a toolbox of techniques and strategies, -with some insights into their strengths and weaknesses, -so that you can make decisions on your own. +=== The Proof Is in the Pudding: Using Docker to Catch Final Bugs -=== The Proof Is in the Pudding: Using Staging to Catch Final Bugs +Remember the deployment checklist from <>? +Let's see if it can't come in useful today! -((("debugging", "server-side", "using staging sites", tertiary-sortas="staging sites", id="DBserstag21"))) -((("staging sites", "catching final bugs with", id="SScatch21"))) -They're all very well for running the FTs locally, -but how would they work against the staging server? -Let's try to deploy our site. -Along the way we'll catch an unexpected bug (that's what staging is for!), -and then we'll have to figure out a way of managing the database on the test server: - -[role="against-server small-code"] -[subs="specialcharacters,quotes"] ----- -$ pass:quotes[*ansible-playbook --user=elspeth -i staging.ottg.co.uk, infra/deploy-playbook.yaml.yaml -vv*] - -PLAYBOOK: deploy-playbook.yaml.yaml *********************************************** -1 plays in infra/deploy-playbook.yaml.yaml -[...] ----- - - -Here's what happens when we run the functional tests: - -[role="against-server small-code"] -[subs="specialcharacters,macros"] ----- -$ pass:quotes[*TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests*] - -====================================================================== -ERROR: test_logged_in_users_lists_are_saved_as_my_lists (functional_tests.test_ -my_lists.MyListsTest.test_logged_in_users_lists_are_saved_as_my_lists) - --------------------------------------------------------------------- -Traceback (most recent call last): - File "...goat-book/functional_tests/test_my_lists.py", line 34, in -test_logged_in_users_lists_are_saved_as_my_lists - self.wait_to_be_logged_in(email) -[...] -selenium.common.exceptions.NoSuchElementException: Message: Unable to locate -element: Log out - - -====================================================================== -FAIL: test_login_using_magic_link (functional_tests.test_login.LoginTest) - --------------------------------------------------------------------- -Traceback (most recent call last): - File "...goat-book/functional_tests/test_login.py", line 22, in -test_login_using_magic_link - self.wait_for(lambda: self.assertIn( -[...] -AssertionError: 'Check your email' not found in 'Server Error (500)' - - --------------------------------------------------------------------- -Ran 8 tests in 68.602s - -FAILED (failures=1, errors=1) - ----- - -We can't log in--either with the real email system or with our pre-authenticated session. -Looks like our nice new authentication system is crashing the server. - - -Let's practice a bit of production debugging! - - -=== Trying to Repro in Docker - -One of the reasons we went to the trouble of building a Docker image, -was to have a way of simulating what's on the server as closely as possible, locally. -So let's see whether we get the same error if we test against Docker. - -Let's rebuild and start our Docker container locally, +First we rebuild and start our Docker container locally, on port 8888: [subs="specialcharacters,quotes"] @@ -112,7 +38,8 @@ $ *docker build -t superlists . && docker run \ -it superlists* ---- -And now let's see if our errors repro against Docker: + +And now let's do an FT run: [role="small-code"] @@ -128,9 +55,10 @@ AssertionError: 'Check your email' not found in 'Server Error (500)' FAILED (failures=1, errors=1) ---- -Sure enough, same two errors! +We can't log in--either with the real email system or with our pre-authenticated session. +Looks like our nice new authentication system is crashing when we run it in Docker. -// TODO: actually, does this obviate the whole need for running fts against the server? +Let's practice a bit of production debugging! === Inspecting the Docker Container Logs @@ -202,59 +130,17 @@ smtplib.SMTPSenderRefused: (530, b'5.7.0 Authentication Required. [...] ---- That looks like a pretty good clue to what's going on. - -Before we go further, it's worth confirming that the error on the actual server -is the same as the one we see in Docker. - -SSH in to your server and run `docker logs`: - -[role="server-commands"] -[subs="specialcharacters,quotes"] ----- -elspeth@server:$ *docker logs superlists* ----- - -You should see an error like this: -[role="skipme small-code"] -[subs="specialcharacters,quotes"] ----- -❯ ssh elspeth@staging.ottg.co.uk docker logs superlists -[2024-10-30 09:55:08 +0000] [6] [INFO] Starting gunicorn 22.0.0 -[2024-10-30 09:55:08 +0000] [6] [INFO] Listening at: http://0.0.0.0:8888 (6) -[2024-10-30 09:55:08 +0000] [6] [INFO] Using worker: sync -[2024-10-30 09:55:08 +0000] [7] [INFO] Booting worker with pid: 7 -Not Found: /favicon.ico -Not Found: /favicon.ico -Not Found: /favicon.ico -Not Found: /favicon.ico -Not Found: /favicon.ico -Internal Server Error: /accounts/send_login_email -Traceback (most recent call last): - File "/venv/lib/python3.13/site-packages/django/core/handlers/exception.py", - line 55, in inner - response = get_response(request) - File "/venv/lib/python3.13/site-packages/django/core/handlers/base.py", line - 197, in _get_response - response = wrapped_callback(request, *callback_args, **callback_kwargs) - File "/src/accounts/views.py", line 16, in send_login_email - send_mail( - ~~~~~~~~~^ - "Your login link for Superlists", - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -[...] - raise SMTPSenderRefused(code, resp, from_addr) -smtplib.SMTPSenderRefused: (530, b'5.7.0 Authentication Required. [...] ----- +((("", startref="Dockercatch21"))) Sure enough! Good to know our local Docker setup can repro the error on the server. -((("", startref="SScatch21")))((("", startref="DBserstag21"))) === Another Environment Variable In Docker -So, Gmail is refusing to send our emails, is it? Now why might that be? -Ah yes, we haven't told the server what our password is! +So, Gmail is refusing to let us send emails, is it? +Now why might that be? "Authentication Required" you say? +Oh woops, we haven't told the server what our password is! As you might remember from earlier chapters, From a86f317ce2d50aa93feabaf829f339be755af7b5 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 23:10:52 +0000 Subject: [PATCH 47/63] Tests might actually be passing --- chapter_19_spiking_custom_auth.asciidoc | 3 + chapter_23_debugging_prod.asciidoc | 65 +++++++++++++-------- source/chapter_23_debugging_prod/superlists | 2 +- tests/test_chapter_23_debugging_prod.py | 9 +-- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 868fa281..98931247 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -721,6 +721,9 @@ image::images/spike-it-worked-windows.png["screenshot of several windows includi TIP: If you get an `SMTPSenderRefused` error message, don't forget to set the `EMAIL_PASSWORD` environment variable in the shell that's running `runserver`. + Also, if you see a message saying "Application-specific password required.", + that's a Gmail security policy. Follow the link in the error message. + That's pretty much it! diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index c7d80a81..fd151b07 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -6,7 +6,7 @@ This chapter has been recently updated to Django 5, Ansible+Docker, etc. It all works on my machine, as they say! -Let me know if you run into anything strange. +Let me know if you run into anything strange. Feedback and suggestions of any kind appreciated, as always. ******************************************************************************* @@ -36,6 +36,12 @@ $ *docker build -t superlists . && docker run \ -e DJANGO_SECRET_KEY=sekrit \ -e DJANGO_ALLOWED_HOST=localhost \ -it superlists* +[...] + => => naming to docker.io/library/superlists [...] +[2025-01-27 22:37:02 +0000] [7] [INFO] Starting gunicorn 22.0.0 +[2025-01-27 22:37:02 +0000] [7] [INFO] Listening at: http://0.0.0.0:8888 (7) +[2025-01-27 22:37:02 +0000] [7] [INFO] Using worker: sync +[2025-01-27 22:37:02 +0000] [8] [INFO] Booting worker with pid: 8 ---- @@ -138,8 +144,8 @@ Sure enough! Good to know our local Docker setup can repro the error on the ser === Another Environment Variable In Docker -So, Gmail is refusing to let us send emails, is it? -Now why might that be? "Authentication Required" you say? +So, Gmail is refusing to let us send emails, is it? +Now why might that be? "Authentication Required" you say? Oh woops, we haven't told the server what our password is! @@ -209,9 +215,12 @@ IndexError: pop from empty list ---- Well, not a pass, but the tests do get a little further. -It looks like our server _can_ now send emails, -(and the docker log no longer shows any errors), -they're just not appearing in `mail.outbox`. +It looks like our server _can_ now send emails +(if you check the docker logs, you'll see there are no more errors) +But our FT is saying it can't see any emails appearing in `mail.outbox`. + + +==== `mail.outbox` Won't Work Outside Django's Test Environment The reason is that `mail.outbox` is a local, in-memory variable in Django, so that's only going to work when our tests and our server are running in the same process, @@ -221,7 +230,8 @@ When we run against another process, be it Docker or an actual server, we can't access the same `mail.outbox` variable. We need another technique if we want to actually inspect the emails -that the server sends, in our tests against Docker or staging. +that the server sends, in our tests against Docker +(or later, against the staging server). [[options-for-testing-real-email]] @@ -239,7 +249,9 @@ There are a few different ways we could test this: and some APIs for checking what mail has been delivered. 3. We can use an alternative, fake email backend, - whereby Django will save the emails to a file on disk for example, + whereby Django will save the emails to a + https://docs.djangoproject.com/en/5.1/topics/email/#file-backend[file on disk] + for example, and we can inspect them there. 4. Or we could give up on testing email on the server. @@ -272,10 +284,10 @@ and Django has supported sending email for more than a decade, so I think we can afford to say, in this case, that the costs of building testing tools for email outweigh the benefits. -We can already repro the issue we saw on the server in our Docker image, -so I'm going to suggest we stick to using `mail.outbox` when we're running local tests, -and we configure our FTs to just check that the server (or Docker) _seems_ to be able to send email -(in the sense of "not crashing") and we can skip the bit where we check the email contents in our FT. +I'm going to suggest we stick to using `mail.outbox` when we're running local tests, +and we configure our FTs to just check that Docker (or, later, the staging server) +_seems_ to be able to send email (in the sense of "not crashing") +and we can skip the bit where we check the email contents in our FT. Remember, we also have unit tests for the email content! NOTE: I explore some of the difficulties involved in getting @@ -297,7 +309,7 @@ Here's where we can put an early return in the FT: ) ) - if self.against_server: + if self.test_server: # Testing real email sending from the server is not worth it. return @@ -309,7 +321,7 @@ Here's where we can put an early return in the FT: This test will still fail if you don't set `EMAIL_PASSWORD` to a valid value in Docker or on the server, so that's good enough for now. -Here's how we populate the `.against_server` attribute: +Here's how we populate the `.test_server` attribute: [role="sourcecode"] @@ -320,14 +332,16 @@ Here's how we populate the `.against_server` attribute: class FunctionalTest(StaticLiveServerTestCase): def setUp(self): self.browser = webdriver.Firefox() - if test_server := os.environ.get("TEST_SERVER"): - self.against_server = True - self.live_server_url = "http://" + test_server - else: - self.against_server = False + self.test_server = os.environ.get("TEST_SERVER") # <1> + if self.test_server: + self.live_server_url = "http://" + self.test_server ---- ==== +<1> We upgrade `test_server` to being an attribute on the test object, + so we can access it in various places in our tests. + We'll see this come in useful later too! + And you can confirm that the FT will fail if you don't set `EMAIL_PASSWORD` in Docker. @@ -361,7 +375,7 @@ that looks up EMAIL_PASSWORD in our local environment: [role="sourcecode dofirst=ch23l012-1"] -.infra/deploy-playbook.yml (ch23l012) +.infra/deploy-playbook.yaml (ch23l012) ==== [source,python] ---- @@ -743,9 +757,9 @@ Perhaps a little ascii-art diagram will help: | server_tools | --> | ssh | -> | docker exec | --> | ./manage.py create_session | | .create_session_on_server | +-----+ +-------------+ | (on server) | | (locally) | +------------------------------+ -+----------------------------+ ++----------------------------+ ---- - + .An Alternative For Managing Test Database Content: Talking Directly to the DB @@ -773,7 +787,6 @@ In any case, let's see if it works. First, locally, to check that we didn't break anything: -[role="dofirst-ch21l022"] [subs="specialcharacters,macros"] ---- $ pass:quotes[*python src/manage.py test functional_tests.test_my_lists*] @@ -834,8 +847,10 @@ Hooray! If you try running the tests twice, you'll run into this error: -[subs="specialcharacters,quotes"] +[role="against-server small-code"] +[subs=""] ---- +$ TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests django.db.utils.IntegrityError: UNIQUE constraint failed: accounts_user.email ---- @@ -867,7 +882,7 @@ And let's add the call to `reset_database()` in our base test `setUp()` method: ==== [source,python] ---- -from .server_tools import reset_database #<1> +from .container_commands import reset_database #<1> [...] class FunctionalTest(StaticLiveServerTestCase): diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 1b0004f4..10a48bdf 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 1b0004f4943d56d401551c1ff941b9cb025e78ab +Subproject commit 10a48bdf638a08412a8b0779a8ac37115c474a48 diff --git a/tests/test_chapter_23_debugging_prod.py b/tests/test_chapter_23_debugging_prod.py index 8ad9c6ff..605dfb49 100644 --- a/tests/test_chapter_23_debugging_prod.py +++ b/tests/test_chapter_23_debugging_prod.py @@ -13,11 +13,12 @@ def test_listings_and_commands_and_output(self): self.parse_listings() # sanity checks - self.assertEqual(self.listings[0].type, "against staging") + self.assertEqual(self.listings[0].type, "docker run tty") self.assertEqual(self.listings[1].type, "output") # skips - # self.skip_with_check(45, "commit changes first") + self.skip_with_check(1, "naming to docker") + if DO_SERVER_COMMANDS: self.replace_command_with_check( 13, @@ -36,9 +37,9 @@ def test_listings_and_commands_and_output(self): # hack fast-forward if os.environ.get("SKIP"): - self.pos = 10 + self.pos = 14 self.sourcetree.run_command( - "git switch {}".format(self.sourcetree.get_commit_spec("ch17l004")) + "git checkout {}".format(self.sourcetree.get_commit_spec("ch23l009")) ) # if DO_SERVER_COMMANDS: From 5830532d92d1dfd3bcd4d70e231cd1d2a6500040 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 23:37:10 +0000 Subject: [PATCH 48/63] flesh out the unique=true and reset db stuff at the end --- chapter_23_debugging_prod.asciidoc | 49 ++++++++++++++++++--- source/chapter_23_debugging_prod/superlists | 2 +- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index fd151b07..2ca2501c 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -180,7 +180,7 @@ Now let's pass that env var thru to our docker container using one more `-e` fla this one fishing the env var out of the shell we're in: -[subs="attributes+,quotes"] +[subs="specialcharacters,quotes"] ---- $ *docker build -t superlists . && docker run \ -p 8888:8888 \ @@ -781,6 +781,7 @@ and that's an approach I've used successfully in the past (see eg https://www.co but it's still fiddly to get right and usually requires writing your own SQL. ********************************************************************** + === Testing the Management Command In any case, let's see if it works. @@ -795,7 +796,31 @@ OK ---- -Next, against the server. +Next, against Docker. Rebuild first: + +[subs="specialcharacters,quotes"] +---- +$ *docker build -t superlists . && docker run \ + -p 8888:8888 \ + --mount type=bind,source=./src/db.sqlite3,target=/src/db.sqlite3 \ + -e DJANGO_SECRET_KEY=sekrit \ + -e DJANGO_ALLOWED_HOST=localhost \ + -e EMAIL_PASSWORD \ + -it superlists* +---- + +And then we run the FT that uses our fixture, against Docker: + +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests.test_my_lists*] + +[...] +OK +---- + + +And now against the server. First, re-deploy to make sure our [role="against-server"] @@ -845,12 +870,15 @@ Hooray! === Test Database Cleanup -If you try running the tests twice, you'll run into this error: +One more thing to be aware of: now that we're running against a real database, +we don't get cleanup for free any more. +If you try running the tests twice--locally or against Docker, +you'll run into this error: -[role="against-server small-code"] -[subs=""] +[subs="specialcharacters,macros"] ---- -$ TEST_SERVER=staging.ottg.co.uk python src/manage.py test functional_tests +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests.test_my_lists*] +[...] django.db.utils.IntegrityError: UNIQUE constraint failed: accounts_user.email ---- @@ -899,6 +927,15 @@ class FunctionalTest(StaticLiveServerTestCase): If you try to run your tests again, you'll find they pass happily. +[role="dofirst-ch23l021"] +[subs="specialcharacters,macros"] +---- +$ pass:quotes[*TEST_SERVER=localhost:8888 python src/manage.py test functional_tests.test_my_lists*] +[...] + +OK +---- + [role="pagebreak-before less_space"] .Warning: Be Careful Not to Run Test Code Against the Production Server! diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 10a48bdf..829023f0 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 10a48bdf638a08412a8b0779a8ac37115c474a48 +Subproject commit 829023f0ab4045b08acd7ebfde1067a0d8a21ff5 From d9327203ed704e97d6b85eeb38597f8353eb1bf0 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 23:39:34 +0000 Subject: [PATCH 49/63] bump django and selenium, fix ch1 test hopefully --- chapter_01.asciidoc | 4 ++-- pre-requisite-installations.asciidoc | 2 +- source/chapter_23_debugging_prod/superlists | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/chapter_01.asciidoc b/chapter_01.asciidoc index 2c109f5a..fbdff8e6 100644 --- a/chapter_01.asciidoc +++ b/chapter_01.asciidoc @@ -108,9 +108,9 @@ $ pass:quotes[*python functional_tests.py*] Traceback (most recent call last): File "...goat-book/functional_tests.py", line 4, in browser.get("http://localhost:8000") - File ".../selenium/webdriver/remote/webdriver.py", line 393, in get + File ".../selenium/webdriver/remote/webdriver.py", line 454, in get self.execute(Command.GET, {"url": url}) - File ".../selenium/webdriver/remote/webdriver.py", line 384, in execute + File ".../selenium/webdriver/remote/webdriver.py", line 429, in execute self.error_handler.check_response(response) File ".../selenium/webdriver/remote/errorhandler.py", line 232, in check_response diff --git a/pre-requisite-installations.asciidoc b/pre-requisite-installations.asciidoc index 7bceba3d..a618ed1d 100644 --- a/pre-requisite-installations.asciidoc +++ b/pre-requisite-installations.asciidoc @@ -381,7 +381,7 @@ Collecting selenium Downloading selenium-4.24.0-py3-none-any.whl (6.5 MB) ---------------------------------------- 6.5/6.5 MB 6.3 MB/s eta 0:00:00 Installing collected packages: django, selenium -Successfully installed [...] django-5.1.4 [...] selenium-4.26.0 [...] +Successfully installed [...] django-5.1.5 [...] selenium-4.28.1 [...] ---- // CSANAD: The output of the pip installation include the dependencies as well, maybe this way // it would better illustrate that? diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 7b206320..829023f0 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 7b2063201e53ca3b701e631380ff3fdf58a87301 +Subproject commit 829023f0ab4045b08acd7ebfde1067a0d8a21ff5 From 0c8393afd9fc09e82cd181325dfc910fd64f4bf9 Mon Sep 17 00:00:00 2001 From: Harry Date: Mon, 27 Jan 2025 23:51:13 +0000 Subject: [PATCH 50/63] seems like the tree hack is no longer necessary on mac --- chapter_08_prettification.asciidoc | 2 +- chapter_17_javascript.asciidoc | 2 +- tests/book_tester.py | 13 ------------- tests/test_chapter_08_prettification.py | 2 +- 4 files changed, 3 insertions(+), 16 deletions(-) diff --git a/chapter_08_prettification.asciidoc b/chapter_08_prettification.asciidoc index 5d8b5ba9..32b49f01 100644 --- a/chapter_08_prettification.asciidoc +++ b/chapter_08_prettification.asciidoc @@ -1062,7 +1062,7 @@ static/ ├── [...] └── bootstrap.min.js.map -16 directories, 171 files +17 directories, 171 files ---- `collectstatic` has also picked up all the CSS for the admin site. diff --git a/chapter_17_javascript.asciidoc b/chapter_17_javascript.asciidoc index f183cee2..89e06720 100644 --- a/chapter_17_javascript.asciidoc +++ b/chapter_17_javascript.asciidoc @@ -436,7 +436,7 @@ src/lists/static/tests ├── jasmine.js └── jasmine_favicon.png -2 directories, 9 files +3 directories, 9 files ---- We need to go edit the _SpecRunner.html_ file diff --git a/tests/book_tester.py b/tests/book_tester.py index 93066c01..46de0ecf 100644 --- a/tests/book_tester.py +++ b/tests/book_tester.py @@ -86,18 +86,6 @@ def standardise_assertionerror_none(output): return output.replace("AssertionError: None", "AssertionError") -def standardise_tree_dir_count(output): - regex = r"(\d+) directories, (\d+) files" - if sys.platform == "darwin": - if match := re.search(regex, output): - return re.sub( - regex, - f"{int(match.group(1)) -1} directories, {match.group(2)} files", - output, - ) - return output - - def standardise_git_init_msg(output): return output.replace( "Initialized empty Git repository", "Initialised empty Git repository" @@ -444,7 +432,6 @@ def assert_console_output_correct(self, actual, expected, ls=False): actual_fixed = fix_creating_database_line(actual_fixed) actual_fixed = fix_interactive_managepy_stuff(actual_fixed) actual_fixed = standardise_assertionerror_none(actual_fixed) - actual_fixed = standardise_tree_dir_count(actual_fixed) actual_fixed = standardise_git_init_msg(actual_fixed) actual_fixed = wrap_long_lines(actual_fixed) diff --git a/tests/test_chapter_08_prettification.py b/tests/test_chapter_08_prettification.py index bfb8b5f7..309191b4 100644 --- a/tests/test_chapter_08_prettification.py +++ b/tests/test_chapter_08_prettification.py @@ -32,7 +32,7 @@ def test_listings_and_commands_and_output(self): if os.environ.get("SKIP"): self.pos = 55 self.sourcetree.run_command( - "git switch {}".format(self.sourcetree.get_commit_spec("ch08l018")) + "git checkout {}".format(self.sourcetree.get_commit_spec("ch08l018")) ) while self.pos < len(self.listings): From e066f72f16027b45525c627ff4775442d6a3aa11 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 28 Jan 2025 11:13:57 +0000 Subject: [PATCH 51/63] use secret for email password --- .github/workflows/tests.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 58744a98..d22a5a79 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -45,6 +45,7 @@ jobs: env: PY_COLORS: "1" # enable coloured output in pytest + EMAIL_PASSWORD: ${{ secrets.GMAIL_APP_PASSWORD }} steps: - uses: actions/checkout@v4 From cff1e2fa2fd76f0cc64593d2bed4c757520b3638 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 28 Jan 2025 11:29:28 +0000 Subject: [PATCH 52/63] remove ref to 008, get rid of all old commits with experiments now in appendix --- chapter_23_debugging_prod.asciidoc | 2 +- source/chapter_23_debugging_prod/superlists | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/chapter_23_debugging_prod.asciidoc b/chapter_23_debugging_prod.asciidoc index 2ca2501c..2f9da3f7 100644 --- a/chapter_23_debugging_prod.asciidoc +++ b/chapter_23_debugging_prod.asciidoc @@ -296,7 +296,7 @@ NOTE: I explore some of the difficulties involved in getting Here's where we can put an early return in the FT: -[role="sourcecode dofirst-ch23l008"] +[role="sourcecode"] .src/functional_tests/test_login.py (ch23l009) ==== [source,python] diff --git a/source/chapter_23_debugging_prod/superlists b/source/chapter_23_debugging_prod/superlists index 829023f0..93fb750a 160000 --- a/source/chapter_23_debugging_prod/superlists +++ b/source/chapter_23_debugging_prod/superlists @@ -1 +1 @@ -Subproject commit 829023f0ab4045b08acd7ebfde1067a0d8a21ff5 +Subproject commit 93fb750ae32a787c973f76582013c48abd6635cd From d1e66d1cef516f2509731958f26f6479006b6950 Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 28 Jan 2025 16:20:17 +0000 Subject: [PATCH 53/63] feed thru to 24 --- source/chapter_24_outside_in/superlists | 2 +- tests/test_chapter_24_outside_in.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/chapter_24_outside_in/superlists b/source/chapter_24_outside_in/superlists index 20de6bab..c13c58c2 160000 --- a/source/chapter_24_outside_in/superlists +++ b/source/chapter_24_outside_in/superlists @@ -1 +1 @@ -Subproject commit 20de6bab868ac37fa81d4ba45c7f30f05d1e8868 +Subproject commit c13c58c2a0ff31904a1caac661c727f2d6e89a0b diff --git a/tests/test_chapter_24_outside_in.py b/tests/test_chapter_24_outside_in.py index c7145d60..0cd15d90 100644 --- a/tests/test_chapter_24_outside_in.py +++ b/tests/test_chapter_24_outside_in.py @@ -28,8 +28,8 @@ def test_listings_and_commands_and_output(self): if os.environ.get("SKIP"): self.pos = 28 self.sourcetree.run_command( - "git switch {}".format( - self.sourcetree.get_commit_spec("ch19l011"), + "git checkout {}".format( + self.sourcetree.get_commit_spec("ch22l011"), ) ) From fa877108385c37c415bcc4aba661e5cbd7baf0ff Mon Sep 17 00:00:00 2001 From: Harry Date: Tue, 28 Jan 2025 16:23:37 +0000 Subject: [PATCH 54/63] feed thru commits to end --- source/chapter_25_CI/superlists | 2 +- source/chapter_26_page_pattern/superlists | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/chapter_25_CI/superlists b/source/chapter_25_CI/superlists index 08f0e411..d40d71e9 160000 --- a/source/chapter_25_CI/superlists +++ b/source/chapter_25_CI/superlists @@ -1 +1 @@ -Subproject commit 08f0e4110a2c105f7c04a212f3bb32fb96e98a6a +Subproject commit d40d71e9074be4f4697e464519ff9fdb0efa9fcd diff --git a/source/chapter_26_page_pattern/superlists b/source/chapter_26_page_pattern/superlists index a63f3df8..536a2d75 160000 --- a/source/chapter_26_page_pattern/superlists +++ b/source/chapter_26_page_pattern/superlists @@ -1 +1 @@ -Subproject commit a63f3df84f4a5fb3329f93796dc07b758de3c3b9 +Subproject commit 536a2d75feff00cc33578963e692ff2ea5b7b924 From ea7996392ba1709cf7d6d900d1dbe9d878d3afbc Mon Sep 17 00:00:00 2001 From: David <11855766+DavidATN@users.noreply.github.com> Date: Tue, 28 Jan 2025 19:26:18 -0600 Subject: [PATCH 55/63] Update HTML tutorial link pre-requisite-installations.asciidoc Removed link to webplatform.org (discontinued in 2015) and replaced with the new URL webplatform.org redirects to (MDN) --- pre-requisite-installations.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pre-requisite-installations.asciidoc b/pre-requisite-installations.asciidoc index a618ed1d..3065271f 100644 --- a/pre-requisite-installations.asciidoc +++ b/pre-requisite-installations.asciidoc @@ -64,7 +64,7 @@ Again, have a look at the three books I recommended previously if you're in any ((("HTML", "tutorials")))I'm also assuming you have a basic grasp of how the web works--what HTML is, what a POST request is, and so on. If you're not sure about those, you'll need to -find a basic HTML tutorial; there are a few at http://www.webplatform.org/. If +find a basic HTML tutorial; there are a few at https://developer.mozilla.org/Learn_web_development. If you can figure out how to create an HTML page on your PC and look at it in your browser, and understand what a form is and how it might work, then you're probably OK. From 383592de67812d47169bc7702b2f0440d82ec92c Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 09:17:58 +0000 Subject: [PATCH 56/63] fix in 25 --- chapter_25_CI.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chapter_25_CI.asciidoc b/chapter_25_CI.asciidoc index 838a3cf0..c051fcd4 100644 --- a/chapter_25_CI.asciidoc +++ b/chapter_25_CI.asciidoc @@ -718,7 +718,7 @@ Took 27ms to run 2 tests. 1 passed, 1 failed. All right! Let's unbreak that, commit and push the runner, and then add it to our [keep-together]#Jenkins# build: -[role="dofirst-ch23l020 skipme"] +[role="skipme"] [subs="specialcharacters,quotes"] ---- $ *git checkout lists/static/list.js* From bd6236f4d3e88b91528cbbfcfa36ca2c0b869475 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 09:22:29 +0000 Subject: [PATCH 57/63] attempted fix in 9 --- chapter_09_docker.asciidoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/chapter_09_docker.asciidoc b/chapter_09_docker.asciidoc index bcceab9c..63c19d68 100644 --- a/chapter_09_docker.asciidoc +++ b/chapter_09_docker.asciidoc @@ -1070,8 +1070,8 @@ The following additional packages will be installed: libbrotli1 libcurl4 libldap-2.5-0 libldap-common libnghttp2-14 libpsl5 [...] root@5ed84681fdf8:/src# pass:quotes[*curl -iv http://localhost:8888*] -* Trying 127.0.0.1:8888... -* Connected to localhost (127.0.0.1) port 8888 +* Trying [...] +* Connected to localhost [...] > GET / HTTP/1.1 > Host: localhost:8888 > User-Agent: curl/8.6.0 @@ -1148,11 +1148,11 @@ to "Empty reply": // TODO: CI consistently says "connection reset by peer", // locally it's empty reply, no matter what curl version -[role="ignore-errors skipme"] +[role="ignore-errors"] [subs="specialcharacters,macros"] ---- $ pass:quotes[*curl -iv localhost:8888*] -* Trying 127.0.0.1:8888... +* Trying [...] * Connected to localhost (127.0.0.1) port 8888 > GET / HTTP/1.1 > Host: localhost:8888 From 1a4819a82f598a92e62382510e85cc885e707fda Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 12:02:14 +0000 Subject: [PATCH 58/63] add david to acks --- acknowledgments.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acknowledgments.asciidoc b/acknowledgments.asciidoc index 57d4e566..5ea514d9 100644 --- a/acknowledgments.asciidoc +++ b/acknowledgments.asciidoc @@ -97,7 +97,7 @@ Sorcha, TJ, Ignacio, Roel, Justyna, Nathan, Andrea, Alexandr, bilyanhadzhi, mosegontar, sfarzy, henziger, hunterji, das-g, juanriaza, GeoWill, Windsooon, gonulate, Margie Roswell, Ben Elliott, Ramsay Mayka, peterj, 1hx, Wi, Duncan Betts, Matthew Senko, Neric "Kasu" Kaz, Dominic Scotto, Andrey Makarov, -Stephanie Goulet, +Stephanie Goulet, David Carter, and many, many more. === Additional Thanks for the Third Edition From cffc3c8033447bc1095bb111fbec0b3c8ba17055 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 12:16:46 +0000 Subject: [PATCH 59/63] try to fix another curl thing in 9 --- chapter_09_docker.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/chapter_09_docker.asciidoc b/chapter_09_docker.asciidoc index 63c19d68..1e41ba89 100644 --- a/chapter_09_docker.asciidoc +++ b/chapter_09_docker.asciidoc @@ -1245,8 +1245,8 @@ We can verify it's working with `curl`: [subs="specialcharacters,macros"] ---- $ pass:quotes[*curl -iv localhost:8888*] -* Trying 127.0.0.1:8888... -* Connected to localhost (127.0.0.1) port 8888 +* Trying [...] +* Connected to localhost [...] [...] From e0fa329ed10a74cd6bf85c856bbba4356f1a4578 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 15:07:54 +0000 Subject: [PATCH 60/63] ok just skip that one --- chapter_09_docker.asciidoc | 3 ++- tests/test_chapter_09_docker.py | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/chapter_09_docker.asciidoc b/chapter_09_docker.asciidoc index 1e41ba89..a3fda4fd 100644 --- a/chapter_09_docker.asciidoc +++ b/chapter_09_docker.asciidoc @@ -1148,7 +1148,7 @@ to "Empty reply": // TODO: CI consistently says "connection reset by peer", // locally it's empty reply, no matter what curl version -[role="ignore-errors"] +[role="ignore-errors skipme"] [subs="specialcharacters,macros"] ---- $ pass:quotes[*curl -iv localhost:8888*] @@ -1162,6 +1162,7 @@ $ pass:quotes[*curl -iv localhost:8888*] * Empty reply from server * Closing connection curl: (52) Empty reply from server + ---- NOTE: Depending on your system, instead of `(52) Empty reply from server`, diff --git a/tests/test_chapter_09_docker.py b/tests/test_chapter_09_docker.py index b2bece12..f6952686 100644 --- a/tests/test_chapter_09_docker.py +++ b/tests/test_chapter_09_docker.py @@ -34,11 +34,13 @@ def test_listings_and_commands_and_output(self): if os.environ.get("SKIP"): # self.pos = 8 # self.pos = 18 - self.pos = 60 + self.pos = 36 + # self.pos = 60 self.sourcetree.run_command( - # "git switch {}".format(self.sourcetree.get_commit_spec("ch09l001")) - # "git switch {}".format(self.sourcetree.get_commit_spec("ch09l003")) - "git switch {}".format(self.sourcetree.get_commit_spec("ch09l008")) + # "git checkout {}".format(self.sourcetree.get_commit_spec("ch09l001")) + "git checkout {}".format(self.sourcetree.get_commit_spec("ch09l005")) + # "git checkout {}".format(self.sourcetree.get_commit_spec("ch09l003")) + # "git checkout {}".format(self.sourcetree.get_commit_spec("ch09l008")) ) print(f"Running in: {self.sourcetree.tempdir}") # vm_restore = "MANUAL_2" From 0189c674263f6db07ffdc82189e1e5472e34947e Mon Sep 17 00:00:00 2001 From: Harry Percival Date: Wed, 29 Jan 2025 16:04:37 +0000 Subject: [PATCH 61/63] Update chapter_15_simple_form.asciidoc --- chapter_15_simple_form.asciidoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/chapter_15_simple_form.asciidoc b/chapter_15_simple_form.asciidoc index e17a81bc..1efe8a60 100644 --- a/chapter_15_simple_form.asciidoc +++ b/chapter_15_simple_form.asciidoc @@ -1204,6 +1204,10 @@ using a form is a nice way of encapsulating that logic. // (https://docs.djangoproject.com/en/5.1/topics/testing/tools/#the-test-client) // for this kind of thing, which doesn't have browser validation built in. // So we could use that if we really wanted to test the server side validation. +// +// [Update]: I just clocked that we are doing exactly this in test_views.py. Might be worth just calling it out a bit more? +// + // // This might be a good opportunity to mention that as an option // (even if we don't go into detail here) From 79df1f7b3825e10d8473995b7662fe45b63cb372 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 16:26:50 +0000 Subject: [PATCH 62/63] a few comments from tr pr descriptions --- chapter_15_simple_form.asciidoc | 8 ++++++ chapter_16_advanced_forms.asciidoc | 9 +++++-- chapter_17_javascript.asciidoc | 8 ++++++ chapter_19_spiking_custom_auth.asciidoc | 36 +++++++++++++++++++++++++ todos.txt | 4 +++ 5 files changed, 63 insertions(+), 2 deletions(-) diff --git a/chapter_15_simple_form.asciidoc b/chapter_15_simple_form.asciidoc index 1efe8a60..a0f5bbc0 100644 --- a/chapter_15_simple_form.asciidoc +++ b/chapter_15_simple_form.asciidoc @@ -18,6 +18,14 @@ and making sure each of them tests only one thing at a time. // - that we're indeed gonna do be doing some refactor // and in such a case functional tests shield us. +// DAVID: Keeping the test suite passing + +// One of the best things I've taken from our recent TDD dojos is the idea of +// keeping the test suite passing during refactors, even to the point of doing +// apparently counter-intuitive things such as copy-pasting code. In this chapter +// we have the test suite failing for quite a while (even to the point where we're +// making git commits while the suite is failing). + === Moving Validation Logic into a Form TIP: In Django, a complex view is a code smell. diff --git a/chapter_16_advanced_forms.asciidoc b/chapter_16_advanced_forms.asciidoc index 7d3615dd..5266a810 100644 --- a/chapter_16_advanced_forms.asciidoc +++ b/chapter_16_advanced_forms.asciidoc @@ -24,6 +24,11 @@ Make sure you take a quick look at the and the <> at the end. // DAVID: should this be 'silliness'? That's the word you use in the aside. +// DAVID: A general point: I feel like just because we're doing TDD +// doesn't mean we can't occasionally start up the application +// and use it to figure out what's happening. +// It feels like a long time since we've done that! + === Another FT for Duplicate Items @@ -866,7 +871,7 @@ classes. Although it would have been nice to minimise hand-written HTML and use Django instead, it seems like we need to bring back our custom `` and add a few attributes manually: -// JAN: Can't you simply add the end version of the code below? These Git views are awful to read and even worse for copy&paste ... +// JAN: Can't you simply add the end version of the code below? These Git views are awful to read and even worse for copy&paste ... // CSANAD: I actually like these. They show quite clearly what to remove and what to add and IMO the reader also learns more from // typing these rather than just copying and pasting. If they really want to just copy and paste, then they could just open the // book-example repo anyway. @@ -905,7 +910,7 @@ use Django instead, it seems like we need to bring back our custom ==== <1> We hand-craft the `` and the most important custom setting will be its - `class`. + `class`. <2> As you can see, we can use conditionals even for providing additional `class` -es.footnote:[ We've split the input tag across multiple lines so it fits nicely on the screen. diff --git a/chapter_17_javascript.asciidoc b/chapter_17_javascript.asciidoc index 9ebb8986..58ea88e4 100644 --- a/chapter_17_javascript.asciidoc +++ b/chapter_17_javascript.asciidoc @@ -49,6 +49,14 @@ NOTE: I'm going to assume you know the basics of JavaScript syntax. // CSANAD: maybe we could also mention MDN // https://developer.mozilla.org/en-US/docs/Web/JavaScript#for_complete_beginners +// DAVID: I have never before written a test in Javascript so I think this +// chapter is really important, at least to give some flavour of what it's like. + +// DAVID: I think it would be improved with a high level overview of how +// Jasmine works - I got it in the end, +// but it would have been helpful at the beginning. + + === Starting with an FT diff --git a/chapter_19_spiking_custom_auth.asciidoc b/chapter_19_spiking_custom_auth.asciidoc index 3f99eed6..2875a3a4 100644 --- a/chapter_19_spiking_custom_auth.asciidoc +++ b/chapter_19_spiking_custom_auth.asciidoc @@ -38,6 +38,38 @@ We'll use something fun called passwordless auth instead. Django's default auth module is ready and waiting for you. It's nice and straightforward, and I'll leave it to you to discover on your own.) +//// +DAVID +I like the way in this chapter you give a flavour of what it's like to give +something a try without TDD - and then return to it. + +But - of all the chapters I've reviewed so far, I think this would most benefit +with some more work. For me, reading the spike comes down to copy-pasting code +that isn't really explained, and I don't think feel carried along with your +thought process for that part of the chapter. So I think it would work better +to move more of the explanation to the spike phase. + +It also might work better to conduct the spike a little differently. For +example: + +You could start with laying out the high level design, with a sequence diagram. +Then you could stub out the user interface (without, say, any actual email - it +could just print the link out to the terminal). Get us to click around and +actually run it. Then you could look at the User model. You could first look at +the representation of the User and talk about why you don't want to use +Django's built in. Then you could look at the Token, whether it could be a +field on the User model versus separate, versus foreign key, versus not stored +in the database at all and use a signed token instead. Finally you could figure +out the actual sending of the email, maybe entirely separate by doing it from +the Django shell. + +Once you've done all that design thinking, then it could be time to go back and +TDD it...but it will make a lot more sense to the reader about the thought +process you've been on. Use django.contrib.auth less? + +Side note: I wonder if it is worth experimenting with using django.contrib.auth even less for this use case, it feels like a slightly awkward fit and a bit difficult to understand what is going on. Then again, maybe not. +//// + [role="pagebreak-before less_space"] === Passwordless Auth With "Magic Links" @@ -379,6 +411,8 @@ on the staging server as well. ==== Storing Tokens in the Database +// CSANAD (transcribed) you should probably hash the tokens + ((("authentication", "storing tokens in databases"))) ((("tokens"))) How are we doing? Let's review where we're at in the process: @@ -958,7 +992,9 @@ selenium.common.exceptions.NoSuchElementException: Message: Unable to locate element: input[name=email]; [...] [...] ---- + // JAN: I see the following exception: ModuleNotFoundError: No module named 'accounts', not the selenium one. We should remove all changes from settings.py/urls.py as well +// HARRY - i think this is bc jan missed the 'git switch main' or earlier commit The first thing it wants us to do is add an email input element. Bootstrap has some built-in classes for navigation bars, so we'll use them, and include a diff --git a/todos.txt b/todos.txt index 5d349653..7b3199bb 100644 --- a/todos.txt +++ b/todos.txt @@ -3,6 +3,10 @@ # Later +* hash the tokens + + + ## switch to postgres - do it in deploy chaps From 77dc01ea4c934fcfa97fdb795f5d52a25e8cab16 Mon Sep 17 00:00:00 2001 From: Harry Date: Wed, 29 Jan 2025 16:47:06 +0000 Subject: [PATCH 63/63] extra comment from ds --- chapter_22_fixtures_and_wait_decorator.asciidoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/chapter_22_fixtures_and_wait_decorator.asciidoc b/chapter_22_fixtures_and_wait_decorator.asciidoc index 60222918..a8ec3821 100644 --- a/chapter_22_fixtures_and_wait_decorator.asciidoc +++ b/chapter_22_fixtures_and_wait_decorator.asciidoc @@ -32,6 +32,9 @@ TIP: Don't overdo de-duplication in FTs. One of the benefits of an FT is that it can catch strange and unpredictable interactions between different parts of your application. +// DAVID: This chapter feels a little short compared to some of the others, +// but otherwise looks good. + === Skipping the Login Process by Pre-creating a Session