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

Don't crash if an item has no _fixtureinfo attribute #2789

Merged
merged 1 commit into from
Sep 19, 2017
Merged

Don't crash if an item has no _fixtureinfo attribute #2789

merged 1 commit into from
Sep 19, 2017

Conversation

obestwalter
Copy link
Member

@obestwalter obestwalter commented Sep 18, 2017

fixes #2788

Simplest possible way of making that error go away. I have obviously no idea what I am doing though.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @obestwalter, that's definitely a good start!

We should add a regression test for it though. I think you can add a new test in testing/python/show_fixtures_per_test.py that contains a docstring with a doctest and a text file, I suggest:

def test_doctest_items(testdir):
    testdir.makepyfile('''
        def foo():
            """
            >>> 1 + 1
            2
            """
    ''')
    testdir.maketxtfile('''
        >>> 1 + 1
        2
    ''')
    result = testdir.runpytest("--fixtures-per-test", "--doctest-modules", "--doctest-glob=*.txt", "-v")
    assert result.ret == 0

    result.stdout.fnmatch_lines([
        '*collected 2 items*',
    ])

This fails for me with the same error you got originally.

Also, please add a new towncrier entry, I suggest 2788.bugfix:

Fix crash when using ``--fixtures-per-test`` in directories with doc test items (docstrings or text files).

@@ -1003,11 +1003,15 @@ def write_fixture(fixture_def):
tw.line(' no docstring available', red=True)

def write_item(item):
name2fixturedefs = item._fixtureinfo.name2fixturedefs
Copy link
Member

Choose a reason for hiding this comment

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

This is just me and a question of preference, but I try to narrow down my try/except blocks as much as possible, so I would write instead:

try:
    name2fixturedefs = item._fixtureinfo.name2fixturedefs
except AttributeError:
    return

if not name2fixturedefs:
    return

But really it is up to you. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. will do that.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 91.808% when pulling ed2a7cf on Avira:master into bf77daa on pytest-dev:master.

@obestwalter
Copy link
Member Author

Thanks, will fix it up properly tomorrow :)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I like the small refactorings you did there. 👍

* doxtests don't seem to have this attribute, so nothing will be written in that case.
* tried to be a good boy scout and tidied up surrounding code a bit (comments, shadowed/unused names, removed random new lines, naming things)
@obestwalter
Copy link
Member Author

obestwalter commented Sep 19, 2017

Thanks.

Bit of a mess up now though: I want to learn to have a cleaner commit history so I squashed the changes, but this has closed this PR. Need to learn how to do this properly ...

@obestwalter obestwalter reopened this Sep 19, 2017
@obestwalter
Copy link
Member Author

obestwalter commented Sep 19, 2017

ah just reopening contains the single commit then ... phew

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 91.828% when pulling bf77daa on Avira:master into bf77daa on pytest-dev:master.

@nicoddemus
Copy link
Member

I want to learn to have a cleaner commit history so I squashed the changes,

Did you do it locally? I do it frequently and it doesn't need to close the PR. Here's how I usually do it:

git rebase -i ... # squash squash squash
git push --force

@obestwalter
Copy link
Member Author

obestwalter commented Sep 19, 2017

Yes, locally: I did a soft reset to the last commit before my changes, stashed the changes, then did a hard reset to that commit, then a force push and then unstashed, committed and pushed the changes again ... that was the first thing I could think of how to do this ... Until now I have only really worked on projects where nobody cares much about git history cleanliness, so I never bothered and stacked up commit on commit - working on tox now though I start to care so I have to up my game there :)

Will do some reading about how to do this without upsetting the github side of things.

@obestwalter
Copy link
Member Author

o.k. the way you did it looks much easier anyway - so:

git rebase -i origin/master~<wherever I want to go back to> master
git push origin +master

@nicoddemus
Copy link
Member

...reset to that commit, then a force push...

OK, this is what closed the PR then: this ended up pushing a branch with no changes compared to master. A little weird that this closes the existing PR, but at least it is consistent given that you can't create a PR without any changes compared to the target branch.

@obestwalter
Copy link
Member Author

Makes sense.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 91.828% when pulling 2802135 on Avira:master into bf77daa on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 91.828% when pulling 2802135 on Avira:master into bf77daa on pytest-dev:master.

@nicoddemus
Copy link
Member

Unless somebody wants more time to review this I will merge this later today! 👍

@nicoddemus nicoddemus merged commit 966391c into pytest-dev:master Sep 19, 2017
@nicoddemus
Copy link
Member

Thanks @obestwalter!

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.

pytest --fixtures-per-test -> 'DoctestItem' object has no attribute '_fixtureinfo'
3 participants