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

Fixes # 3592 - Clarify Fixtures' Documentation #3642

Merged
merged 9 commits into from Jul 12, 2018

Conversation

Projects
None yet
3 participants
@caramelomartins
Copy link
Contributor

commented Jun 30, 2018

Hey,

I'm starting to use pytest and thought I'd give a small help. I've noticed #3592 hasn't been fixed yet. I decided to go the suggested way with smtp_connection because it felt clearer. This PR only features documentation changes.

Since the page was quite large and I am inexperienced, I'll review it again tomorrow but this gives an opportunity for sharing what has already been done.

Fixes #3592

Checklist:

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

Not Applicable:

  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

I didn't add myself to AUTHORS because I don't believe it warrants that.

Hope everything is okay. Let me know if changes are needed.

Regards,
Hugo

@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2018

I see the builds have failed. Will take a look at why linting failed.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 30, 2018

Hi @caramelomartins, thanks for the contribution.

You can run the linting checks by installing pre-commit, see the contributing guide for details. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 30, 2018

Thanks @caramelomartins, but I think there are a few spots where it still uses smtp instead of smtp_connection. If you are on linux, could you please run tox -e regen to generate the examples again? The problems will be clearer that way.

@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

@nicoddemus I'll take a look. There were some that I left because I thought they made sense but I'll review it again (using tox -e regen like you suggest).

EDIT: Just fixing a typo.

@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

@nicoddemus I've noticed there were a lot of missing smtp references, specially in the zone that explains yield. I've ran tox -e regen and reviewed it. How is it looking now? Thanks for your patience. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Thanks @caramelomartins! I took the liberty of running pre-commit in order to fix linting on CI. I will review the rest of the PR as soon as I can. 👍

@coveralls

This comment has been minimized.

Copy link

commented Jul 4, 2018

Coverage Status

Coverage increased (+0.05%) to 92.649% when pulling aa47b64 on caramelomartins:master into 593b451 on pytest-dev:master.

@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

@nicoddemus Thank you for running the linter. 🏅 Let me know if you find any more troubles. 👍

@nicoddemus
Copy link
Member

left a comment

Thanks @caramelomartins!

I couldn't find the source of the problems I found here. I will take another look as soon as I can.

@@ -456,8 +457,8 @@ server URL in its module namespace::

smtpserver = "mail.python.org" # will be read by smtp fixture

def test_showhelo(smtp):
assert 0, smtp.helo()
def test_showhelo(smtp_connection):

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 4, 2018

Member

This code seems right, but is raising a NameError below, not sure why...

> assert 0 # for demo purposes
E assert 0
def test_ehlo(smtp_connection):
> response, msg = smtp_connection.ehlo()

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 4, 2018

Member

Here it is also weird, the fixture value is actually the function...

> assert 0 # for demo purposes
E assert 0
def test_noop(smtp_connection):
> response, msg = smtp_connection.noop()

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 4, 2018

Member

Here it is also weird, the fixture value is actually the function...

> assert b"smtp.gmail.com" in msg
E AssertionError: assert b'smtp.gmail.com' in b'mail.python.org\nPIPELINING\nSIZE 51200000\nETRN\nSTARTTLS\nAUTH DIGEST-MD5 NTLM CRAM-MD5\nENHANCEDSTATUSCODES\n8BITMIME\nDSN\nSMTPUTF8'
def test_ehlo(smtp_connection):
> response, msg = smtp_connection.ehlo()

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 4, 2018

Member

Here it is also weird, the fixture value is actually the function...

> assert 0 # for demo purposes
E assert 0
def test_noop(smtp_connection):
> response, msg = smtp_connection.noop()

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 4, 2018

Member

Here it is also weird, the fixture value is actually the function...

@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@nicoddemus I'll explore the problems you highlight once I can. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

@caramelomartins thanks! I've been meaning to take a look myself, but didn't find the time yet. 👍

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Hi @caramelomartins,

Found the problem, it was just a print that was using the old name (4dfe2ee).

I regenerated the docs myself and now this is good to go.

Thanks again for the PR! 👍

@nicoddemus nicoddemus changed the title [WIP] Fixes # 3592 - Clarify Fixtures' Documentation Fixes # 3592 - Clarify Fixtures' Documentation Jul 11, 2018

@nicoddemus nicoddemus merged commit 6c37132 into pytest-dev:master Jul 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@caramelomartins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Ah, shame I couldn't catch it @nicoddemus .

Thanks for the patience and for the awesome work on pytest!

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.