Skip to content
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

Testing in the tutorial #934

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Testing in the tutorial #934

merged 1 commit into from
Mar 7, 2017

Conversation

butla
Copy link
Contributor

@butla butla commented Oct 27, 2016

This aims to solve #253.
It's currently a work in progress.

@kgriffs Do you think I'm going in the right way with incorporating TDD into the tutorial?
Doesn't it break the flow or tone too much?
Also, do you have some deadline for the tutorial? Because I probably won't be able to work on it for a week now.

@codecov-io
Copy link

codecov-io commented Nov 6, 2016

Codecov Report

Merging #934 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #934   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          31     31           
  Lines        2107   2031   -76     
  Branches      338    328   -10     
=====================================
- Hits         2107   2031   -76
Impacted Files Coverage Δ
falcon/api.py 100% <0%> (ø)
falcon/util/misc.py 100% <0%> (ø)
falcon/api_helpers.py 100% <0%> (ø)
falcon/errors.py 100% <0%> (ø)
falcon/testing/client.py 100% <0%> (ø)
falcon/request.py 100% <0%> (ø)
falcon/response.py 100% <0%> (ø)
falcon/routing/compiled.py 100% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d43d295...c5a8f4c. Read the comment docs.

@butla butla force-pushed the master branch 3 times, most recently from d17080f to 7863f8f Compare November 8, 2016 18:06
@butla
Copy link
Contributor Author

butla commented Nov 8, 2016

Pep8 checks for import order on my example files are failing. I can make them pass, but the order they require is bizarre (from examples/look/tests/test_app.py):

from unittest.mock import call, mock_open # standard library

from look.app import api # my own module

import msgpack # third party code
import pytest # still third party, in alphabetical order
import falcon # third party in reverse alphabetical order?
from falcon import testing

As I understand, PEP8 encourages standard/third-party/own (and alphabetical order in each section) import order.
Can someone offer some insight?

@butla butla force-pushed the master branch 2 times, most recently from 4c68eef to e50bfbd Compare November 9, 2016 16:21
@butla
Copy link
Contributor Author

butla commented Nov 9, 2016

@kgriffs Ok, I think I'm done. That's a big chunk of docs there... Hope you like it.

But I still have the problem with pep8 mentioned above.

Oh, and I've pointed to my own library in the section about functional tests, is that ok?

@butla
Copy link
Contributor Author

butla commented Nov 30, 2016

@kgriffs Any update?

Copy link
Member

@kgriffs kgriffs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, this is an impressive tutorial, and will be a super helpful addition to the docs. Thank you! I added a number of suggestions inline, but most of them are fairly nitpicky. Thanks again!

powered REPL that is good to have in your toolbox when
exploring a new library.
`bpython <http://bpython-interpreter.org/>`_ or
`ptpython <https://github.com/jonathanslenders/ptpython>`_ are another
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: "are another" ==> "are other"

@@ -144,6 +164,8 @@ and add the following to it:

def on_get(self, req, resp):
resp.body = '{"message": "Hello world!"}'
# This line can be ommited, because 200 is the default code falcon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


Gunicorn has still limitation that is not working on Windows. If you are Windows user you can use Waitress server instead Gunicorn

.. code:: bash

$ pip install waitress
$ waitress-serve --port=8000 app:api
$ waitress-serve --port=8000 look.app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I modified this manually to resolve a merge conflict with the latest code in master.

Testing your application
------------------------

Up to this point we didn't care about tests, but when creating applications
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could make the second clause in this sentence more compelling. What do you think about something like the following?

Up to this point we didn't care about tests, but fully exercising your code is critical to creating robust applications with a great user experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Up to this point we didn't care about tests, but when creating applications
that will be used by someone you should have those. So, as an exercise, we'll create
the next piece of code in accordance with `Test Driven Development
<http://www.obeythetestinggoat.com/book/praise.harry.html>`_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this guide is Django-centric, is there a more generic TDD reference that we might use instead? Alternatively, it may make sense to include a note about how the linked guide uses Django for its examples, but the principles and workflow are the same, regardless of what web framework you use.

(Don't get me wrong, Django is a great choice for many use cases. I just want to make sure we are taking the reader's context into consideration.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... this one is the best I know, even though I'm not a fan of Django :) I'll add a note with explanations.

they don't check anything more than ``test_posted_image_gets_saved``. But we can
do that only because we have a very simple application logic.

Normally, you would check component integration and few more meaningful logic paths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and few more meaningful logic paths

===>

and all primary logic paths

do that only because we have a very simple application logic.

Normally, you would check component integration and few more meaningful logic paths
through the entire application with functional tests, and leave the bulk of testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"through" ===> "throughout"


Normally, you would check component integration and few more meaningful logic paths
through the entire application with functional tests, and leave the bulk of testing
to unit tests. But the actual ratio of unit/functional tests depend entirely on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"depend" ===> "depends"

"on applications problem domain and will vary." ===> "on each application's problem domain, and will vary."

to unit tests. But the actual ratio of unit/functional tests depend entirely on
applications problem domain and will vary.

After this section we won't be doing TDD anymore as you should have a good grip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"we won't be doing TDD anymore as you" ==> "we'll omit the TDD instructions, as you..."

applications problem domain and will vary.

After this section we won't be doing TDD anymore as you should have a good grip
of testing in Falcon by now. Instead, we'll focus on showcasing some more of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in Falcon" ===> "Falcon applications"

@butla
Copy link
Contributor Author

butla commented Jan 18, 2017

@kgriffs Good that it's finally going somewhere :-) I'm quite busy now, but I'll try to go over your suggestions in a week or two.

@kgriffs
Copy link
Member

kgriffs commented Feb 3, 2017

@butla do you think you'll have some time to work on this over the next few days? It would be great to get this in the docs for 1.2 if we can.

@butla
Copy link
Contributor Author

butla commented Feb 3, 2017

@kgriffs I may be able to work on this on Tuesday, if not, then definitely on Wednesday :)
Is that okay?

@kgriffs
Copy link
Member

kgriffs commented Feb 3, 2017

@butla cool, sounds good. Thanks!

@butla
Copy link
Contributor Author

butla commented Feb 15, 2017

@kgriffs That was quite a thorough review you did there :)
I think I've fixed everything you wanted, but there are still some issues remaining:

  • using only PNG throughout the tutorial (I'm in favor of PNG, there was something odd about jpeg mimetype and .jpg files, but I don't remember anymore...)
  • sending the images through redirecting to "http" everywhere (I think I should do it, do you?)
  • the issue of strange import order in my code samples (more explanations in the comment somewhere above)

After all of those are resolved, I'll try to squash my commits and rebase onto the current master.

@butla
Copy link
Contributor Author

butla commented Feb 16, 2017

Ok, so I've already did the changes with PNG as the image type everywhere and with sending the images with redirect to http.

I'll also try to squash and rebase my changes onto master on a different branch.

@butla
Copy link
Contributor Author

butla commented Feb 16, 2017

All right, so if my changes are OK, I'll forcibly push my "squash" branch onto my "master", and this will be ready to merge... Once we resolve the issue of the strange import order in the code samples :) Or should I just put the imports in the order the tool wants it?

Oh, and one more thing, during squashing I've rephrased two sentences that looked clunky to me.
Here it is on the diff (the other change was done in the upstream)

butla@b2:~/development/falcon$ git diff master squash -- docs/user/tutorial.rst
diff --git a/docs/user/tutorial.rst b/docs/user/tutorial.rst
index 8025490..9a0a0f2 100644
--- a/docs/user/tutorial.rst
+++ b/docs/user/tutorial.rst
@@ -109,9 +109,9 @@ let's use something that you would actually deploy in production.

     $ pip install gunicorn
     $ gunicorn look.app
-
-Gunicorn has still limitation that is not working on Windows.
-If you are Windows user you can use Waitress server instead Gunicorn
+
+If you are a Windows user, then use Waitress instead of Gunicorn, since the latter
+doesn't work under Windows.

 .. code:: bash

@@ -1065,7 +1065,10 @@ such as `IPython <http://ipython.org/>`_ or

 Also, don't be shy about pulling up Falcon's source code on GitHub or in your
 favorite text editor. The team has tried to make the code as straightforward
-and readable as possible; where other documentation may fall short, the code basically
-"can't be wrong."
-
+and readable as possible; where other documentation may fall short, the code
+basically "can't be wrong."

+A number of Falcon add-ons, templates, and complimentary packages are
+available for use in your projects. We've listed several of these on the
+`Falcon wiki <https://github.com/falconry/falcon/wiki>`_ as a starting
+point, but you may also wish to search PyPI for additional resources.

@kgriffs
Copy link
Member

kgriffs commented Mar 3, 2017

Oh, and one more thing, during squashing I've rephrased two sentences that looked clunky to me.

Those changes LGTM. Thanks!

@kgriffs
Copy link
Member

kgriffs commented Mar 3, 2017

As I understand, PEP8 encourages standard/third-party/own (and alphabetical order in each section) import order.

When the Falcom framework tests run with flake8-import-order and --application-import-names=falcon the import falcon statement is expected to be grouped with the local module imports... Perhaps this is what is happening with examples/look/tests ?

All right, so if my changes are OK, I'll forcibly push my "squash" branch onto my "master", and this will be ready to merge

@butla Everything LGTM. Let's go ahead get this squashed. Thanks!

@butla
Copy link
Contributor Author

butla commented Mar 7, 2017

@kgriffs Ok, that shed some light on the order but I was still left with a strange order (builtin -> local -> external -> falcon) in the test for the sample app.

import io
from unittest.mock import call, MagicMock, mock_open

import look.app
import look.images

import msgpack
import pytest

import falcon
from falcon import testing
import falcon.request_helpers

Anyway, I made the imports look like that, rebased onto the current master, and put that on my master, so it should be ready to merge.

@kgriffs
Copy link
Member

kgriffs commented Mar 7, 2017

OK, I think this is ready to merge! I can play around with the pep8 tox env to see if I can fix the import issue. I'll probably need to create some additional tox env(s) just for testing the example.

@kgriffs kgriffs merged commit 73507b1 into falconry:master Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants