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

Add parameter info to fixture assert_num_queries to display additiona… #663

Merged
merged 2 commits into from
Oct 30, 2018

Conversation

MrtnBckr
Copy link
Contributor

…l message on failure.

@codecov-io
Copy link

codecov-io commented Oct 15, 2018

Codecov Report

Merging #663 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #663      +/-   ##
=========================================
+ Coverage   94.38%   94.4%   +0.02%     
=========================================
  Files          33      33              
  Lines        1834    1841       +7     
  Branches      156     157       +1     
=========================================
+ Hits         1731    1738       +7     
  Misses         78      78              
  Partials       25      25
Flag Coverage Δ
#dj110 86.25% <100%> (+0.05%) ⬆️
#dj111 88.64% <100%> (+0.04%) ⬆️
#dj18 87.23% <100%> (+0.04%) ⬆️
#dj19 86.04% <100%> (+0.05%) ⬆️
#dj20 86.52% <100%> (+0.05%) ⬆️
#dj21 82.56% <100%> (+0.06%) ⬆️
#djmaster 82.56% <100%> (+0.06%) ⬆️
#mysql_innodb 86.74% <100%> (+0.05%) ⬆️
#mysql_myisam 86.58% <100%> (+0.05%) ⬆️
#postgres 89.89% <100%> (+0.03%) ⬆️
#py27 91.58% <100%> (+0.03%) ⬆️
#py34 86.04% <100%> (+0.05%) ⬆️
#py35 86.25% <100%> (+0.05%) ⬆️
#py36 87.45% <100%> (+0.04%) ⬆️
#py37 82.56% <100%> (+0.06%) ⬆️
#sqlite 88.59% <100%> (+0.04%) ⬆️
#sqlite_file 86.04% <100%> (+0.05%) ⬆️
Impacted Files Coverage Δ
tests/test_fixtures.py 99.6% <100%> (ø) ⬆️
pytest_django/fixtures.py 97.25% <100%> (+0.03%) ⬆️

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 9804e61...93a9522. Read the comment docs.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

I am not sure if this is really needed, given that you could also just add a comment above / next to the assertion?!

@MrtnBckr
Copy link
Contributor Author

@blueyed Yes, you could. But that way the comment won't be part of the error message supplied by giving -v parameter. For better understanding of the error, I like to see some hint above the queries about what was expected.

@anrie
Copy link
Contributor

anrie commented Oct 29, 2018

@blueyed

I know that it's possible to customize the error message like this:

assert a % 2 == 0, "value was odd, should be even"

But this doesn't seem to be possible, if you do the assertion via a context manager?
Do you see any other way to provide extra context to the error output in this case?

@blueyed
Copy link
Contributor

blueyed commented Oct 29, 2018

I've meant that you would see it from the traceback already if it was a comment.
I am not completely against this, but do not find it that useful, and think that it only complicates things.

assert a % 2 == 0, "value was odd, should be even"

Would be nice if this pattern could be used, but it would also miss the original info then, i.e. the number of queries.

@MrtnBckr
Copy link
Contributor Author

Yes, you are right, that it is visible in the traceback. But not visible enough for me. I like the spot on top of the error message above the queries. And I don't think, it is very complicated. We use this parameter in our edited version of assert_num_queries in our projects and it can be a big help for developers running in this errors, who are new to the project.

@blueyed
Copy link
Contributor

blueyed commented Oct 29, 2018

Ok.

What do you think about this on top?

diff --git c/docs/helpers.rst i/docs/helpers.rst
index afe2f89..5e8cb17 100644
--- c/docs/helpers.rst
+++ i/docs/helpers.rst
@@ -278,24 +278,23 @@ Example
         assert settings.USE_TZ
 
 
-``django_assert_num_queries``
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
 .. fixture:: django_assert_num_queries
 
+``django_assert_num_queries``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. py:function:: django_assert_num_queries(connection=None, info=None)
+
+  :param connection: optional non-default DB connection
+  :param str info: optional info message to display on failure
+
 This fixture allows to check for an expected number of DB queries.
 
-It wraps `django.test.utils.CaptureQueriesContext`.  A non-default DB
-connection can be passed in using the `connection` keyword argument, an
-additional info message which is displayed on fail can be passed in using
-the `info` keyword argument, and it will yield the wrapped
+It wraps `django.test.utils.CaptureQueriesContext` and yields the wrapped
 CaptureQueriesContext instance.
 
 
-Example
-"""""""
-
-::
+Example usage::
 
     def test_queries(django_assert_num_queries):
         with django_assert_num_queries(3) as captured:
@@ -306,20 +305,21 @@ Example
         assert 'foo' in captured.captured_queries[0]['sql']
 
 
+.. fixture:: django_assert_max_num_queries
+
 ``django_assert_max_num_queries``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-.. fixture:: django_assert_max_num_queries
+.. py:function:: django_assert_num_queries(connection=None, info=None)
+
+  :param connection: optional non-default DB connection
+  :param str info: optional info message to display on failure
 
 This fixture allows to check for an expected maximum number of DB queries.
 
 It is a specialized version of :fixture:`django_assert_num_queries`.
 
-
-Example
-"""""""
-
-::
+Example usage::
 
     def test_max_queries(django_assert_max_num_queries):
         with django_assert_max_num_queries(3):

@MrtnBckr
Copy link
Contributor Author

I'm totally okay with that! Much more understandable now.

@blueyed
Copy link
Contributor

blueyed commented Oct 30, 2018

Added.

btw: got confused when rebasing this: you had the same commit almost the same, and resolved the conflict in a merge commit. For the future, please try to push a fixup instead.

@MrtnBckr
Copy link
Contributor Author

Thank you. And I will push a fixup in future PRs. Sry for confusing you.

@blueyed blueyed merged commit 4a356c1 into pytest-dev:master Oct 30, 2018
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.

4 participants