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

Plasma subclass tests as separate test files #489

Merged

Conversation

Projects
None yet
4 participants
@ritiek
Copy link
Contributor

ritiek commented Jun 6, 2018

This PR just modifies the directory structure for plasma subclass tests. We used to have tests for both (plasmapy/classes/sources/) PlasmaBlob and Plasma3D in plasmapy/classes/tests/test_Plasma.py. Now these tests have a separate file in plasmapy/classes/sources/tests/.

This is the way SunPy does it and personally looks neater to me, we test for main Plasma classes in plasmapy/classes/tests/ and our Plasma subclasses in plasmapy/classes/sources/tests/.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Jun 6, 2018

Hello @ritiek! Thanks for updating your pull request.

Congratulations! There are no PEP8 issues in this pull request. 😸

Comment last updated on June 07, 2018 at 18:01 Hours UTC
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #489 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   92.94%   92.95%   +<.01%     
==========================================
  Files          67       68       +1     
  Lines        6508     6513       +5     
==========================================
+ Hits         6049     6054       +5     
  Misses        459      459
Impacted Files Coverage Δ
plasmapy/classes/sources/tests/test_plasmablob.py 100% <100%> (ø)
plasmapy/classes/sources/tests/test_plasma3d.py 100% <100%> (ø)

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 65e1662...02f62fc. Read the comment docs.

@ritiek ritiek force-pushed the ritiek:improve-plasma-directory-structure branch from 88edc8f to 7fcafc2 Jun 6, 2018

@ritiek

This comment has been minimized.

Copy link
Contributor Author

ritiek commented Jun 6, 2018

This is weird. I have no idea why this decreases coverage.

@StanczakDominik

This comment has been minimized.

Copy link
Member

StanczakDominik commented Jun 6, 2018

I'll take a look in the evening. Probably nothing too serious.

@StanczakDominik

This comment has been minimized.

Copy link
Member

StanczakDominik commented Jun 6, 2018

The weird thing is, I took a look at this locally, comparing the changes between master and this branch and it does decrease coverage! Those few lines are indeed getting uncovered for some reason. I'll have to do some more digging.

@StanczakDominik

This comment has been minimized.

Copy link
Member

StanczakDominik commented Jun 6, 2018

This is so weird. It looks like the same tests are running. I think there are two options here. Either

  • to figure out why this is happening, make another branch with these same changes, only commit them more gradually, pushing every time to see which particular change kills coverage
  • decide it's not worth it to worry about 0.4%, write a few more tests on top of what we have now to cover the missing lines aaaaand merge away.
@ritiek

This comment has been minimized.

Copy link
Contributor Author

ritiek commented Jun 7, 2018

Aww yiss, we did it! So, it was just nasty __init__.py missing from classes/sources/tests/. In fact, classes/sources/tests/* tests weren't even running (and lol it still gave us so good coverage %).

One thing I noticed that those tests did run with missing __init__.py when running pytest directly pytest plasmapy/classes/ but not with python setup.py test. Gotta remember this now.

This should be ready to merge if everything looks good.

@StanczakDominik

This comment has been minimized.

Copy link
Member

StanczakDominik commented Jun 8, 2018

Darn. We might be missing a bunch of other tests if they don't run with setup.py test without __init__.py! I'll have to take a look. Thank you!

Also, that's one elegant change :D

@StanczakDominik StanczakDominik merged commit 51072c3 into PlasmaPy:master Jun 8, 2018

5 checks passed

ci/circleci: test-html Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 92.94%)
Details
codecov/project 92.95% (+<.01%) compared to 65e1662
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ritiek ritiek deleted the ritiek:improve-plasma-directory-structure branch Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.