Skip to content

Clean-up Tests #329

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

Clean-up Tests #329

wants to merge 14 commits into from

Conversation

vmuriart
Copy link
Contributor

@vmuriart vmuriart commented Jan 22, 2017 •
edited
Loading

What does this implement/fix? Explain your changes.

  • Remove dependency on six
  • Fixes broken tests
  • Improves Py2/Py3 testing
  • Adds unused modules
  • Comments broken modules
  • Simplifies tests
  • Renames tests
  • Many pep8 clean-ups

Does this close any currently open issues?

No

Any other comments?

Recommend squash&merge. Left as separate commits for anyone that wants to see the logic for some of the changes.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • [N/A ] Updated the CHANGELOG

@vmuriart vmuriart self-assigned this Jan 22, 2017
@vmuriart vmuriart force-pushed the unittests branch 8 times, most recently from 91d7629 to 4fcfc78 Compare January 22, 2017 22:35
@den-run-ai
Copy link
Contributor

any reasoning for removing dependency on six?

@vmuriart
Copy link
Contributor Author

It wasn't necessary. This way the compatibility module is frozen for our tests. ie, won't be surprised if six introduces a bug in the future.

@vmuriart vmuriart force-pushed the unittests branch 3 times, most recently from d642268 to e791e88 Compare January 23, 2017 05:09
@den-run-ai
Copy link
Contributor

@vmuriart i'm big minus one on removing dependency from six, it is third most dependent package on pypi:

http://kgullikson88.github.io/blog/pypi-analysis.html

it is very unlikely that we are smarter than all these folks using six.

i do not want to maintain our own subset of six, unless there is unresolved issue with six affecting us.

@vmuriart
Copy link
Contributor Author

@denfromufa. We barely used six. Mostly we used six to check if we were in PY2 or PY3 and then created our own compatibility variable. Since we don't support py25, py32, we don't need six.u(...). The only remaining usage for six were two calls to indexbytes.

I refactored all the checks into the compat module. I dont think its worth to keep six only for indexbytes.

@den-run-ai
Copy link
Contributor

@vmuriart I agree, good catch on six. let's get rid of it. give me some time to review this PR after 2.2 release.

@vmuriart
Copy link
Contributor Author

@denfromufa no worries. I want to merge #301 first, and then work on this one. There's going to be a merge conflict after 2.2.0, but i should be able to fix it quickly.

@vmuriart vmuriart mentioned this pull request Jan 26, 2017
Closed
@vmuriart vmuriart force-pushed the unittests branch 2 times, most recently from 6204c29 to a8c7b15 Compare January 29, 2017 06:42
@vmuriart vmuriart mentioned this pull request Jan 29, 2017
Merged
filmor
filmor reviewed Jan 30, 2017
View reviewed changes
Copy link
Member

@filmor filmor left a comment

Choose a reason for hiding this comment

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

Very nice :)

I read through, but I'll have to test this on my machine before approving.

@codecov-io
Copy link

codecov-io commented Jan 30, 2017 •
edited by codecov bot
Loading

Codecov Report

Merging #329 into master will increase coverage by 0.35%.

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage    61.1%   61.45%   +0.35%     
==========================================
  Files          61       61              
  Lines        5324     5324              
  Branches      897      897              
==========================================
+ Hits         3253     3272      +19     
+ Misses       1832     1817      -15     
+ Partials      239      235       -4
Impacted Files Coverage Δ
src/runtime/modulefunctionobject.cs 54.54% <ø> (-27.28%) ❌
src/runtime/classbase.cs 77.14% <ø> (-2.86%) ❌
src/runtime/pythonengine.cs 56.46% <ø> (+0.68%) ✅
src/runtime/metatype.cs 67.69% <ø> (+2.3%) ✅
src/runtime/interop.cs 95.73% <ø> (+2.36%) ✅
src/runtime/classmanager.cs 92.99% <ø> (+3.82%) ✅
src/runtime/methodobject.cs 64.38% <ø> (+4.1%) ✅
src/runtime/propertyobject.cs 65.15% <ø> (+10.6%) ✅

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 8035d67...b4e8d97. Read the comment docs.

@vmuriart
Clean-up tests & remove warnfilter
0ea7455 
Ensure they are using the same functions in py2/py3
Ensure py2/py3 compat
Misc. cleanup
Add _compat

The unittest module has been also updated to use the 'default' filter while running tests.
DeprecationWarnings are ignored by default

https://docs.python.org/3.3/library/warnings.html#updating-code-for-new-versions-of-python
https://docs.python.org/3.3/library/warnings.html#default-warning-filters
@vmuriart
Sanitize how to run tests
0e26b1a 
* Remove unsupported entry points
* Adds reference to Python.Test by default for all tests
* Remove redundant add_reference
* Avoid some implicit AddReferences that were being done

Not all tests added reference to Python.Test consistently. Solve this by making `run_test` the only supported method.
vmuriart added 11 commits January 31, 2017 08:49
@vmuriart
Remove dependency on six
8f29544 
Remove six.u(...), six.b(...)

Not needed since dropped older python versions
@vmuriart
Clean-up imports
30470bb 
- Fix py2/py3 range/zip behavior
  - Ensure same functions being used
- Fix Exception/System.Exception name clashes
@vmuriart
Fix nameclash object
d79db2d
@vmuriart
Refactor utility functions
072219c
@vmuriart
Rename tests
5206a46
@vmuriart
Refactor assertRaises
2ef3041 
test_exceptions: Add exc_info test & fix Exception nameclash
@vmuriart
Format for pep8 and linters
8c3fe2b 
Fix max min nameclash
@vmuriart
Fix tests comparison to None, True, False, isinstance
c583053 
- Replace type(()) with tuple to clarify intent
@vmuriart
Rename test fixtures
4350ac3 
Can mess with test discovery
@vmuriart
Remove unused using from testing and format
2ae78cc
@vmuriart
Use unittest.skip(...)
b4e8d97
@vmuriart
Copy link
Contributor Author

@filmor did you get a chance to check this out?

@filmor
Copy link
Member

filmor commented Jan 31, 2017

Yep, seems fine on my machine ;)

@vmuriart vmuriart closed this in 905884c Jan 31, 2017
@vmuriart vmuriart deleted the unittests branch January 31, 2017 20:47
@filmor
Copy link
Member

filmor commented Jan 31, 2017

You probably should have squashed this before merging :)

@vmuriart
Copy link
Contributor Author

I was debating it, there are some benefits to squashing (compress history), but at the same time there are some losses when going back threw commit history and trying to understand why a file was added/removed.

Personally I clean-up the history before submitting the pull request. That way it has my logical units of change, without having my 10,000 commits saying "FIXUP! typo" 👍

@vmuriart vmuriart mentioned this pull request Feb 8, 2017
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor filmor approved these changes

@tonyroberts tonyroberts Awaiting requested review from tonyroberts

@den-run-ai den-run-ai Awaiting requested review from den-run-ai

Assignees

@vmuriart vmuriart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vmuriart @den-run-ai @codecov-io @filmor