Merge pull request #374 from Xronophobe/csanad-review--chapter-26

Csanád's review of chapter 26

Author: hjwp

chapter_26_page_pattern.asciidoc

@@ -105,14 +105,27 @@ It can be used as an alternative to the `tearDown` function as a way of
 cleaning up resources used during the test.  It's most useful when the resource
 is only allocated halfway through a test, so you don't have to spend time in
 `tearDown` figuring out what does or doesn't need cleaning up.
+// CSANAD: since in the next paragraph (and in the docs) we say addCleanup is
+// run after tearDown I find it unclear what the benefit is, or what the
+// last sentence means (no need to figure out what needs to be cleaned up).
+// From the docs, this seems to be the clear benefit of using addCleanup.
+// "If setUp() fails, meaning that tearDown() is not called, then any cleanup
+// functions added will still be called."
+// Maybe this explanation could be clearer why we need it here.
 
 `addCleanup` is run after `tearDown`, which is why we need that
 `try/except` formulation for `quit_if_possible`; whichever of `edith_browser`
 and `oni_browser` is also assigned to `self.browser` at the point at which the
 test ends will already have been quit by the `tearDown` function.
+// CSANAD: despite this explanation I still find it unclear why we need
+// try/except or how it would work (e.g. what would prevent the browser from
+// closing (that would end up raising an exception) or why we need to call
+// `browser.quit` from both the cleanUp here, and from the base class
+// FunctionalTest's tearDown method.
 
 We'll also need to move `create_pre_authenticated_session` from
 'test_my_lists.py' into 'base.py'.
+// CSANAD: I would add why: "so that we can use it with both FT -s".
 
 OK, let's see if that all works:
 
@@ -463,6 +476,9 @@ If you'd like a bit more help, here's an outline of the steps you could take:
 
 1. We'll need a new section in _list.html_, with, at first,
   a form with an input box for an email address.
+// CSANAD: IMO for a non-native speaker (myself included) this reads a little
+// difficult. Maybe "We'll need a new section in list.html, initially with a
+// form with an input box for an email address." ?
   That should get the FT one step further.
 
 2. Next, we'll need a view for the form to submit to.