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

Nose to pytest transition #183

Merged
merged 6 commits into from Mar 24, 2018
Merged

Conversation

bilderbuchi
Copy link
Contributor

@bilderbuchi bilderbuchi commented Mar 16, 2018

Move from nose to pytest. Fixes #58.

The commit messages have some explanations, basically this was more or less tedious copy/paste. A lot of code was touched because the transition to pytest.raises necessitates increasing indentation for a whole block. (You can inspect changes without considering whitespace by appending ?w=1 to the URL)

Pytest tests all pass, the same number as before with nose. Let's see what the CI says.

This touches a lot of code because pytest.raises
is used in a context manager, so indentation
has to be changed.

Replacement happened with regex from
@raises\((.*)\) to @pytest.mark.xfail(raises=$1)
then from @pytest.mark.xfail\(raises=(.*)\)\ndef(.*)
to def$2\n    with pytest.raises($1):
then manual correction of indentation.
@bilderbuchi
Copy link
Contributor Author

bilderbuchi commented Mar 16, 2018

OK, I just discovered some problems from replacing the eq_ calls, the regex logic was (of course) not smart enough. Funny that the tests should pass, though. :-/ assert statements can be tricky.

@coveralls
Copy link

coveralls commented Mar 16, 2018

Coverage Status

Coverage increased (+0.2%) to 85.29% when pulling 2056008 on bilderbuchi:fix-58 into ffe0a45 on Galvant:master.

@bilderbuchi
Copy link
Contributor Author

bilderbuchi commented Mar 16, 2018

OK, the remaining error in the CI ("E:452,20: Method 'value' has no 'index' member (no-member)") is not even in code I touched, so I am unsure how to fix this. Also, I'm not sure the warning is even correct, as the line in question is

value = valid.index(value) + 2

so index is called on valid, not value, and value is a float, not a method, afaict?

@bilderbuchi
Copy link
Contributor Author

OK, squash one set of spurious warnings by upgrading, get another one. The inconsistent-return-statements warnings could be pylint-dev/pylint#1782, which is still open.

@scasagrande
Copy link
Contributor

Hello there!! This is fantastic, thank you for your hard work! I will take a look at everything this weekend, but a quick glance shows a pretty comprehensive body of work.

Upgrading pylint always seems to bring more issues out with it :)

@bilderbuchi
Copy link
Contributor Author

OK, I cleaned up the commit history and fixed the CI problems, mainly by pinning astroid, too. Ready for review.

@bilderbuchi bilderbuchi changed the title Fix 58 Move from nose to pytest Mar 17, 2018
@bilderbuchi bilderbuchi changed the title Move from nose to pytest Nose to pytest transition Mar 17, 2018
@scasagrande
Copy link
Contributor

I have taken a look, and it seems good from a first pass. I'll check the code out (hopefully today) and give it a whirl just for my sanity.

With pytest I'll probably want to migrate repetitive mock.patch calls to use pytest fixtures to clean things up, but that can be a follow up body of work.

@bilderbuchi
Copy link
Contributor Author

Yeah, I did not do anything to make the tests more "pytest-idiomatic", as that can and should come later.

@scasagrande scasagrande merged commit bb4ab21 into instrumentkit:master Mar 24, 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.

None yet

3 participants