Skip to content

Commit

Permalink
add feedback frm shannon collins for processing
Browse files Browse the repository at this point in the history
  • Loading branch information
hjwp committed Sep 27, 2012
1 parent 001fce3 commit a05a7bf
Showing 1 changed file with 87 additions and 0 deletions.
87 changes: 87 additions & 0 deletions tdd-feedback.txt
@@ -0,0 +1,87 @@
Feedback:
---------

General:
! Your site's 404 shows that debug=True... https://docs.djangoproject.com/en/dev/ref/settings/#debug
+ references to "test server" should be "development server" like the official Django docs, to reduce ambiguity, particularly since "test" is a new term to the typical viewer
- references to 'tests.py' are not descriptive enough- there are at least two files of that name.
+ consider moving the filename accompanying each script box to the top left corner. i was in Part 3 before i even noticed it, and had been guessing which of the `tests.py` to use. In the case of smaller resolutions, it might not be visible until until all the code has been read.
- especially since there is refactoring involved, it might be helpful to have a version of the code that corresponds to the expected results after each lesson (perhaps as a tag, or branch). This could also help a user jump in at any lesson.
- i found the fts to be spotty (i suspect a selenium issue); they would often fail early in an unreproducable manner. After repeatedly hitting the fts, i would be able to power through. Any word on that? Examples:

ERROR: test_voting_on_a_new_poll (fts.tests.PollsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/shannon/work/internal/projects/TDDjangoTutorial/fts/tests.py", line 157, in test_voting_on_a_new_poll
self._setup_polls_via_admin()
File "/home/shannon/work/internal/projects/TDDjangoTutorial/fts/tests.py", line 121, in _setup_polls_via_admin
self.browser.find_element_by_link_text('Add poll').click()
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 237, in find_element_by_link_text
return self.find_element(by=By.LINK_TEXT, value=link_text)
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 671, in find_element
{'using': by, 'value': value})['value']
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 156, in execute
self.error_handler.check_response(response)
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 147, in check_response
raise exception_class(message, screen, stacktrace)
NoSuchElementException: Message: u'Unable to locate element: {"method":"link text","selector":"Add poll"}'


ERROR: test_voting_on_a_new_poll (fts.tests.PollsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/shannon/work/internal/projects/TDDjangoTutorial/fts/tests.py", line 157, in test_voting_on_a_new_poll
self._setup_polls_via_admin()
File "/home/shannon/work/internal/projects/TDDjangoTutorial/fts/tests.py", line 120, in _setup_polls_via_admin
self.browser.find_elements_by_link_text('Polls')[1].click()
IndexError: list index out of range


ERROR: test_voting_on_a_new_poll (fts.tests.PollsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/shannon/work/internal/projects/TDDjangoTutorial/fts/tests.py", line 208, in test_voting_on_a_new_poll
body_text = self.browser.find_element_by_tag_name('body').text
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 309, in find_element_by_tag_name
return self.find_element(by=By.TAG_NAME, value=name)
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 671, in find_element
{'using': by, 'value': value})['value']
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py", line 156, in execute
self.error_handler.check_response(response)
File "/home/shannon/.virtualenvs/tdd/local/lib/python2.7/site-packages/selenium/webdriver/remote/errorhandler.py", line 147, in check_response
raise exception_class(message, screen, stacktrace)
NoSuchElementException: Message: u'Unable to locate element: {"method":"tag name","selector":"body"}'


Part 1
+ some images in /tutorial/1/ are not rendering, but showing something like `.. image:: /static/images/django_admin_login.png`
- don't like the "yes, we really meant it" explanation of INSTALLED_APPS. Prefer something like the "enabled" language in https://docs.djangoproject.com/en/dev/ref/settings/#installed-apps
- `Ran 323 tests in 2.402s` before "Hooray! The joy of unbroken..." should say `Ran 1 test in ...s`
- "It's OK to ignore these for now - we'll deal with templates for 500 errors in a later tutorial." ... i didn't find that later tutorial, but nominate Part 3
- reverse the order of the `Poll` and `admin` imports in `mysite/polls/admin.py` before "If you've done everything right, the polls app folder will look like this:" to match Part 2 and final codebase

Part 2
- i don't think the user was instructed to run syncdb for the new Poll model, at 'Now, when you click the link you should see a menu a bit like this.' The message, "Remember, you may need to syncdb..." comes late
- you might want to explain why 'body' is redefined in the test after every click, for a novice user
+ `https://github.com/hjwp/Test-Driven-Django-Tutorial/blob/master/fts/tests.py` link gives 404. Should be `https://github.com/hjwp/Test-Driven-Django-Tutorial/blob/master/mysite/fts/tests.py`
- grammar: "finally, check it's attributes have been saved" : "its"
- `Ran 326 tests in 2.745s` but it should be `Ran 4 tests ...s`
- "Tune in next week" is not relevant language anymore, since it's already been released.

Part 3
- "Don't forget the import at the top!" - which one? TestCase, Poll?
- "That should give us a folder structure like this:" shows a `fts/test_polls.py` (also referred to in stack trace in Part 2) that doesn't exist. Also, it doesn't show `polls/views.py`, which does
- is the HomePageViewTest called 'test_root_url_shows_all_polls' or 'test_root_url_shows_links_to_all_polls'? i see it referred to both ways.
- 'ViewDoesNotExist: Could not import polls.views.poll. View does not exist in module polls.views.' - i do not get this error when following the instructions, although view does indeed not exist.

Part 4
- grammar: "# and check the check the form" : remove "the check "
- show import for PollVoteForm before `return render(request, 'poll.html', {'poll': poll, 'form': form})`

Part 5
- grammar: `# results. it still says # "100 %: very awesome".` Capitalize "It" and "Very"
- i propose a test-driven way of refactoring tests: have them copy the model tests to an empty `test_models.py`, fail when the imports are missing, then add the necessary imports (rather than copy the whole file and remove parts) because the tests can guide that process. This way, you don't end up with unnecessary imports in the resulting modules (like importing a form in the model tests) although it is a bit more tedious. You don't seem to have a beef with tedium, though...
- Part 4 (and 5) says `test_page_shows_choices_using_form` goes in SinglePollViewTest, but it's in the forms tests, according to the repo
- code underneath `Dealing with POST requests in a view` doesn't indicate that there is still (or could be) a `test_page_shows_poll_title_and_no_votes_message` method before `test_page_shows_choices_using_form`
- grammar: `# check it's votes have gone up by 1` : "its"

0 comments on commit a05a7bf

Please sign in to comment.