Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
vmuriart wants to merge14 commits intopythonnet:masterfromvmuriart:unittests
Closed

Clean-up Tests#329

vmuriart wants to merge14 commits intopythonnet:masterfromvmuriart:unittests

Conversation

@vmuriart
Copy link
Contributor

@vmuriartvmuriart commentedJan 22, 2017•
edited
Loading

What does this implement/fix? Explain your changes.

  • Remove dependency onsix
  • 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?

Recommendsquash&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 toAUTHORS
  • [N/A ] Updated theCHANGELOG

@vmuriartvmuriart self-assigned thisJan 22, 2017
@vmuriartvmuriartforce-pushed theunittests branch 8 times, most recently from91d7629 to4fcfc78CompareJanuary 22, 2017 22:35
@den-run-ai
Copy link
Contributor

any reasoning for removing dependency on six?

@vmuriart
Copy link
ContributorAuthor

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

@vmuriartvmuriartforce-pushed theunittests branch 3 times, most recently fromd642268 toe791e88CompareJanuary 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
ContributorAuthor

@denfromufa. Webarely usedsix. Mostly we usedsix 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 needsix.u(...). The only remaining usage forsix were two calls toindexbytes.

Irefactored all the checks into thecompat module. I dont think its worth to keep six only forindexbytes.

@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
ContributorAuthor

@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.

Copy link
Member

@filmorfilmor 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 commentedJan 30, 2017•
edited by codecovbot
Loading

Codecov Report

Merging#329 intomaster willincrease coverage by0.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 FilesCoverage Δ
src/runtime/modulefunctionobject.cs54.54% <ø> (-27.28%)❌
src/runtime/classbase.cs77.14% <ø> (-2.86%)❌
src/runtime/pythonengine.cs56.46% <ø> (+0.68%)✅
src/runtime/metatype.cs67.69% <ø> (+2.3%)✅
src/runtime/interop.cs95.73% <ø> (+2.36%)✅
src/runtime/classmanager.cs92.99% <ø> (+3.82%)✅
src/runtime/methodobject.cs64.38% <ø> (+4.1%)✅
src/runtime/propertyobject.cs65.15% <ø> (+10.6%)✅

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update8035d67...b4e8d97. Read thecomment docs.

Ensure they are using the same functions in py2/py3Ensure py2/py3 compatMisc. cleanupAdd _compatThe unittest module has been also updated to use the 'default' filter while running tests.DeprecationWarnings are ignored by defaulthttps://docs.python.org/3.3/library/warnings.html#updating-code-for-new-versions-of-pythonhttps://docs.python.org/3.3/library/warnings.html#default-warning-filters
* 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 doneNot all tests added reference to Python.Test consistently. Solve this by making `run_test` the only supported method.
Remove six.u(...), six.b(...)Not needed since dropped older python versions
- Fix py2/py3 range/zip behavior  - Ensure same functions being used- Fix Exception/System.Exception name clashes
test_exceptions: Add exc_info test & fix Exception nameclash
Fix max min nameclash
- Replace type(()) with tuple to clarify intent
Can mess with test discovery
@vmuriart
Copy link
ContributorAuthor

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

@filmor
Copy link
Member

Yep, seems fine on my machine ;)

@vmuriartvmuriart deleted the unittests branchJanuary 31, 2017 20:47
@filmor
Copy link
Member

You probably should have squashed this before merging :)

@vmuriart
Copy link
ContributorAuthor

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" 👍

@vmuriartvmuriart mentioned this pull requestFeb 8, 2017
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@filmorfilmorfilmor approved these changes

@tonyrobertstonyrobertsAwaiting requested review from tonyroberts

@den-run-aiden-run-aiAwaiting requested review from den-run-ai

Assignees

@vmuriartvmuriart

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
[8]ページ先頭
©2009-2025 Movatter.jp