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

Switch to pytest #788

Merged
merged 197 commits into from
Aug 11, 2017
Merged

Switch to pytest #788

merged 197 commits into from
Aug 11, 2017

Conversation

jsmnbom
Copy link
Member

@jsmnbom jsmnbom commented Aug 11, 2017

This is a joint PR between me and @Eldinnie.

This completes the move to the pytest testing framework. Tests are now run using (after installing requirements-dev.txt:

# -v means verbose
$ pytest -v 

With this our coverage once again reaches > 90% and with #778, it should increase even further (this is due to removing de_json tests from the objects mentioned in that issue, since it seemed stupid to port tests that were just gonna get removed)

Other changes

This PR also includes some other changes related to tests without actually being tests

  • Remove travis.py helper script (not needed since folding is now a plugin and pre-commit-hooks and build are tested in test_meta.py)
  • Move converage config (.coveragerc) to setup.cfg for two reasons:
    1. Nicer to have tool configuration conbined in one place
    2. Less files clogging up our root directory (less scrolling for people wanting to read our readme)
  • Exclude a couple of lines from coverage (this was more of a test to see if the pragma: no cover comment worked, but those lines should indeed be excluded from coverage)
  • A few of the attributes of Game and Message now default to list() instead of None since they are indeed lists no matter what
  • Update contributing guide, requirements and travis/appveyor configurations to new tests format
  • Small change to effective_attachment "Because a few of the possible attachments will evaluate to an empty list if not present (photo for instance). This way it covers those too." - @Eldinnie.

Opening a new PR instead of piggy bagging on #757, since it's gonna be squashed and @Eldinnie didn't care for the Github credit (on the projects collaborators view and on his profile), the other alternative was a manually squash a lot of stuff which wouldn't just be quite ugly but also time consuming

@tsnoam tsnoam self-assigned this Aug 11, 2017
@tsnoam tsnoam self-requested a review August 11, 2017 18:20
@Eldinnie Eldinnie mentioned this pull request Aug 11, 2017
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

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

Excellent work!
A few minor comments. Also, as discussed, please have the unitests classes inherit from object.

[report]
omit =
tests/
telegram/vendor/*
Copy link
Member

Choose a reason for hiding this comment

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

removing this file is in order to switch to codecov?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite, it's moved to setup.cfg so it's together with the pytest config

- if [[ $TRAVIS_PYTHON_VERSION != 'pypy'* ]]; then pip install ujson; fi

script:
- python travis.py
- pytest -v --cov=telegram

after_success:
coveralls
Copy link
Member

Choose a reason for hiding this comment

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

aren't we switching out from coveralls?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are, but that's not in this PR

new_class = re.sub(r'self.json_dict', r'json_dict', new_class)

new_text += new_class
new_file = Path('pytests/' + sys.argv[1]).open('w', encoding='UTF-8').write(new_text)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this file in git?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, no we do indeed not

Copy link
Member

Choose a reason for hiding this comment

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

please remove then?


''' Commented out, because it would cause "Too Many Requests (429)" errors.
Copy link
Member

Choose a reason for hiding this comment

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

Should we have this comment copied to the new tests as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's marked as xfail if it raises RetryError or if on pypy (where it's strangely more unstable)

Copy link
Member

Choose a reason for hiding this comment

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

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants