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

Documentation: expansion of Monkeypatch to include mocked classes and dictionaries #5315

Merged
merged 2 commits into from Jun 3, 2019

Conversation

Projects
None yet
4 participants
@EvanKepner
Copy link
Contributor

commented May 27, 2019

Following up on #5250 I've added another expansion to the monkeypatch documentation:

  1. Included the reference output from pytest --fixtures for monkeypatch to list methods.
  2. All Python code-blocks are in code-block RST sections across the file.
  3. Added conceptual scenarios for the major functions to the intro.
  4. Modified the "simple example" to include broader explanatory comments.
  5. Added "Monkeypatching returned objects: building mock classes" section.
  6. Added "Monkeypatching dictionaries" section.
  7. Applied changes from the blacken-docs pre-commit hook.

PR Checklist

  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.

EvanKepner added a commit to EvanKepner/pytest that referenced this pull request May 27, 2019

@codecov

This comment has been minimized.

Copy link

commented May 27, 2019

Codecov Report

Merging #5315 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5315      +/-   ##
==========================================
- Coverage   90.42%   89.96%   -0.47%     
==========================================
  Files         115      115              
  Lines       26260    26260              
  Branches     2588     2588              
==========================================
- Hits        23745    23624     -121     
- Misses       2178     2300     +122     
+ Partials      337      336       -1
Impacted Files Coverage Δ
testing/python/setup_plan.py 20% <0%> (-80%) ⬇️
testing/test_session.py 24.05% <0%> (-75.95%) ⬇️
testing/test_runner_xunit.py 29.82% <0%> (-70.18%) ⬇️
testing/examples/test_issue519.py 33.33% <0%> (-66.67%) ⬇️
testing/logging/test_reporting.py 72.95% <0%> (-27.05%) ⬇️
testing/test_runner.py 34.7% <0%> (-25.17%) ⬇️
testing/python/show_fixtures_per_test.py 85.29% <0%> (-14.71%) ⬇️
testing/test_warnings.py 89.07% <0%> (-9.84%) ⬇️
testing/python/metafunc.py 84.09% <0%> (-7.45%) ⬇️
testing/logging/test_fixture.py 93.1% <0%> (-6.9%) ⬇️
... and 17 more

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 ece774f...2dfbed1. Read the comment docs.

@nicoddemus
Copy link
Member

left a comment

Thanks a lot @EvanKepner, great contribution!

I made a few suggestions, mostly grammar and style, otherwise the docs are expansive with great examples.

Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
always returns ``True``, or return different values from the ``json()`` mocked method
based on input strings.

This mock can be shared across tests using the ``fixture`` syntax:

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member
Suggested change
This mock can be shared across tests using the ``fixture`` syntax:
This mock can be shared across tests using a ``fixture``:

A fixture is not really a new syntax. 😁

This comment has been minimized.

Copy link
@EvanKepner

EvanKepner May 27, 2019

Author Contributor

Applied in 7fa0e04

Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
@@ -90,7 +266,7 @@ to do this using the ``setenv`` and ``delenv`` method. Our example code to test:

.. code-block:: python
# contents of our original code file e.g. code.py
# Contents of our original code file e.g. code.py.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member

I would leave all those "contents -> Contents" changes lone, just for consistency with the rest of the docs.

This comment has been minimized.

Copy link
@EvanKepner

EvanKepner May 27, 2019

Author Contributor

Great catch, also ensured monkeypatch is lowercase everywhere except when it is the first word in the headers.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member

I still see comments starting with an upper case letter... would you mind changing all comments to start with lowercase? This is the pattern we use through the docs and code. 👍

This comment has been minimized.

Copy link
@blueyed

blueyed May 27, 2019

Contributor

I still see comments starting with an upper case letter... would you mind changing all comments to start with lowercase? This is the pattern we use through the docs and code.

That's new to me also - I see that there are comments without punctuation and starting lowercased, but when using punctuation / full sentences they should be capitalized (like above).

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member

full sentences yes, but in one-liners like this we don't use punctuation and start with a lowercase letter (at least that's what I see browsing the docs quickly now)

This comment has been minimized.

Copy link
@blueyed

blueyed May 27, 2019

Contributor

Well, you will also see no typing hints currently - but maybe soon.. ;)
(what I want to say is that style can be improved, and I prefer it like above also)

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member

Well I won't get into that discussion right now then, the current style is fine.

@EvanKepner could you just squash and rebase your commits then? Thanks!

This comment has been minimized.

Copy link
@EvanKepner

EvanKepner May 28, 2019

Author Contributor

No problem at all, squash/rebase coming soon. Also @nicoddemus sorry I misread your original comment and thought you wanted all comment lines capitalized but without punctuation unless part of a larger sentence 😅

I'm a fan of generally strict type hints and full doc-strings for each function. It's a difficult challenge with documentation because you don't want to add additional syntax that could confuse a reader. These tutorial examples are simplified to show how the patching works so I steered towards the least verbose form.

Might be worth an update in the guidance to https://github.com/pytest-dev/pytest/blob/master/CONTRIBUTING.rst#write-documentation if you come to a consensus on standardized documentation style beyond the pre-commit checks for black in the docs. If you're seeking suggestions I like one sentence per line in RST with a hard-wrap (or just a hard-wrap at something sensible) to make suggested changes and PR reviews easier.

@@ -110,7 +286,7 @@ both paths can be safely tested without impacting the running environment:

.. code-block:: python
# contents of our test file e.g. test_code.py
# Contents of our test file e.g. test_code.py.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member

Also, if I'm being pedantic, we usually don't use . at the end of single line comments

This comment has been minimized.

Copy link
@EvanKepner

EvanKepner May 27, 2019

Author Contributor

I changed this throughout the file with regard to the single line comments.

_ = app.create_connection_string()
The modularity of the ``fixture`` syntax gives you the flexibility to define

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus May 27, 2019

Member
Suggested change
The modularity of the ``fixture`` syntax gives you the flexibility to define
The modularity of fixtures gives you the flexibility to define

This comment has been minimized.

Copy link
@EvanKepner

EvanKepner May 27, 2019

Author Contributor

Applied in 7fa0e04 - also realized the second test does not require the mock_test_database fixture so I removed it from the signature - 7fa0e04#diff-425daef754438812ec1a0e4d3aeaeff8L434

@asottile
Copy link
Member

left a comment

just a few small things -- thanks for this writeup!

Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
Show resolved Hide resolved doc/en/monkeypatch.rst Outdated
@asottile
Copy link
Member

left a comment

awesome! thanks for this :)

@EvanKepner

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@nicoddemus @asottile appreciate the fast review! I made the updates, agreed with all suggestions 👍

@asottile

This comment has been minimized.

Copy link
Member

commented May 27, 2019

test failures here are expected -- until #5063 is merged

@nicoddemus
Copy link
Member

left a comment

Besides changing the capitalization of the first letters, could you also please squash your comments and rebase on the latest master? Thanks!

@EvanKepner EvanKepner force-pushed the EvanKepner:master branch from 60e4fd4 to 24c95c7 May 28, 2019

@EvanKepner

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Squashed and rebased to master at ece774f. This includes the prior edits, and I went through and lower-cased all comment lines unless they were part of a sentence.

@EvanKepner

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Realized the change to os.path.expanduser("~/.ssh") actually caused the test example to fail, since expanduser was patched to return "/abc". Added a commit to update to this to use pathlib instead which is more clear and confirmed that it runs correctly.

@EvanKepner

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@nicoddemus checking to make sure there is no need for additional action from me. If more edits are required please let me know. Thanks!

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Hi @EvanKepner,

Thanks for the ping, missed that this was ready. 👍

@nicoddemus nicoddemus merged commit 2a3d643 into pytest-dev:master Jun 3, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
pytest-CI #20190528.3 failed
Details
WIP Ready for review
Details
@EvanKepner

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Excellent - thanks! No worries at all 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.