Merge pull request #247 from seddonym/seddonym-chap-12

Review Chapter 12

Author: hjwp

chapter_12_organising_test_files.asciidoc

@@ -63,6 +63,9 @@ def test_cannot_add_empty_list_items(self):
 ----
 ====
 
+// DAVID: I think we should spell out that you want us to add this to the test file. Also,
+// where? As a method of NewVisitorTestCase?
+
 That's all very well, but before we go any further--our
 functional tests file is beginning to get a little crowded.
 Let's split it out into several files, in which each has a single test method.
@@ -80,6 +83,8 @@ how to get there step by step.
 NOTE: We're back to local development now.
     Double check that the `TEST_SERVER` environment variable is unset in your terminal.
 
+// DAVID: might be nice to remind people how they check this?
+
 ((("unittest module", "skip test decorator")))
 ((("refactoring")))
 ((("decorators", "skip test decorator")))
@@ -99,6 +104,7 @@ from unittest import skip
     def test_cannot_add_empty_list_items(self):
 ----
 ====
+// DAVID: The indentation here looks weird - maybe better to include the class declaration too?
 
 This tells the test runner to ignore this test.
 You can see it works--if we rerun the tests,
@@ -172,6 +178,10 @@ https://www.oreilly.com/library/view/tidy-first/9781098151232/[Tidy First?]
 ((("test files", "splitting FTs into many")))
 We start putting each test into its own class, still in the same file:
 
+// DAVID: I would say this is too big a refactor all in one go.
+// A nicer way to do this would be to break out the base class first as one refactor, moving it into another file.
+// Then when we copy the files we could rename each class and delete the irrelevant methods.
+
 [role="sourcecode"]
 .src/functional_tests/tests.py (ch11l002)
 ====
@@ -205,6 +215,8 @@ class ItemValidationTest(FunctionalTest):
 ----
 ====
 
+// DAVID: I also have check_for_row_in_list_table as a method - maybe I should've removed it?
+
 At this point we can rerun the FTs and see they all still work:
 
 ----
@@ -384,6 +396,7 @@ when we're only interested in a single one.
 Although we need to remember to run all of them now and again, to check for regressions.
 Later in the book we'll set up a Continuous Integration (CI) server to run all the tests automatically,
 for example every time we push to master.
+// DAVID: "master" -> "our main branch"
 For now, a good prompt for running all the tests is "just before you do a commit",
 so let's get into that habit now:
 
@@ -463,6 +476,8 @@ might decide that building a specific helper method is overkill at this stage,
 but it might be nice to have some generic way of saying, in our tests, "wait
 until this assertion passes".  Something like this:
 
+// DAVID: might be worth reminding the reader about that helper method, e.g. what it's called.
+
 [role="sourcecode"]
 .src/functional_tests/test_list_item_validation.py (ch11l009)
 ====
@@ -512,6 +527,8 @@ and we'll adapt it slightly:
 ----
 ====
 
+// DAVID: I have `raise e` from that other method, not `raise`.
+
 <1> We make a copy of the method, but we name it `wait_for`,
     and we change its argument.  It is expecting to be passed a function.
 
@@ -595,6 +612,9 @@ and that can be executed later, and multiple times:
 
 Let's see our funky `wait_for` helper in action:
 
+// DAVID: Might be worth explicitly telling the reader to add that code earlier - I hadn't yet,
+// so I have to go back and find the right snippet.
+
 [subs="macros,verbatim"]
 ----
 $ pass:quotes[*python src/manage.py test functional_tests.test_list_item_validation*]
@@ -862,6 +882,8 @@ Use a tests folder::
     * You probably want a separate test file for each tested source code
       file. For Django, that's typically 'test_models.py', 'test_views.py', and
       'test_forms.py'.
+// DAVID: worth mentioning that mirroring test files to real files can pave the way for automatically running tests
+// relevant to your changes.
     * Have at least a placeholder test for 'every' function and class.
     ((("test files", "organizing and refactoring")))