Skip to content

Csanád's review of chapter 26 #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions chapter_26_page_pattern.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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.
Expand Down
Loading