Move pytest helpers to subdirectory of utils #674

namurphy · 2019-09-05T23:55:16Z

Our pytest helpers functionality has been in a single file in plasmapy.utils, which means that the functionality in there is jumbled together with a bunch of other unrelated things. Putting it in its own subdirectory will help with the organization. I also split the existing functionality in pytest.

I updated the namespaces for run_test, etc. in a bunch of test files throughout the package. I also made a few non-consequential changes in the test files that I noticed, mostly removing unused import statements.

I am planning on doing more refactoring to the pytest helpers functionality, which will enable me to do some more refactoring of tests (discussed in comments in #605). The API for utils.pytest_helpers should be considered unstable at the moment.

pep8speaks · 2019-09-05T23:55:24Z

Hello @namurphy! Thanks for updating your pull request.

In the file plasmapy/utils/pytest_helpers/error_messages.py:

Line 84:22: E251 unexpected spaces around keyword / parameter equals
Line 84:24: E251 unexpected spaces around keyword / parameter equals
Line 85:29: E251 unexpected spaces around keyword / parameter equals
Line 85:31: E251 unexpected spaces around keyword / parameter equals

Comment last updated at 2019-09-06 00:00:51 UTC

codecov · 2019-09-05T23:59:04Z

Codecov Report

Merging #674 into master will increase coverage by 0.07%.
The diff coverage is 90.8%.

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
+ Coverage   94.95%   95.03%   +0.07%     
==========================================
  Files          55       58       +3     
  Lines        4661     4674      +13     
==========================================
+ Hits         4426     4442      +16     
+ Misses        235      232       -3

Impacted Files	Coverage Δ
plasmapy/classes/sources/openpmd_hdf5.py	`80.23% <ø> (ø)`	⬆️
plasmapy/classes/sources/plasma3d.py	`100% <ø> (ø)`	⬆️
plasmapy/physics/__init__.py	`100% <ø> (ø)`	⬆️
plasmapy/utils/pytest_helpers/pytest_helpers.py	`93.3% <100%> (ø)`
plasmapy/utils/__init__.py	`100% <100%> (ø)`	⬆️
plasmapy/classes/sources/plasmablob.py	`100% <100%> (ø)`	⬆️
plasmapy/utils/pytest_helpers/exceptions.py	`100% <100%> (ø)`
plasmapy/utils/pytest_helpers/__init__.py	`100% <100%> (ø)`
plasmapy/atomic/parsing.py	`99.45% <66.66%> (+1.59%)`	⬆️
plasmapy/utils/pytest_helpers/error_messages.py	`89.23% <89.23%> (ø)`
... and 3 more

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 437957b...a2740fc. Read the comment docs.

Our pytest helpers functionality has been in a single file in plasmapy.utils, which means that the functionality in there is jumbled together with a bunch of other unrelated things. Putting it in its own subdirectory will help with the organization. I also split the existing functionality in pytest. I updated the namespaces for run_test, etc. in a bunch of test files throughout the package. I also made a few non-consequential changes in the test files that I noticed, mostly removing unused import statements. I am planning on doing more refactoring to the pytest helpers functionality, which will enable me to do some more refactoring of tests. The API for utils.pytest_helpers should be considered unstable at the moment.

StanczakDominik

Looks Mostly Good To Me! 👍

One point I should probably be raising somewhere else, though: I saw pytest_helpers messing up when plugged into the parallel testing of pytest_xdist somewhere along the last few of my PRs (apologies for not bringing that up sooner) - as this code base grows, we're going to be finding more edge cases... I guess my point is that we should really make sure we aren't reinventing Pytest's own functionalities in here and stick with the "canonical" solutions to problems. I think I remember thinking "this would have been fine with a fixture" in a bunch of places around test_atomic.

I'm deeply sorry for being vague and unhelpful right now >_<

StanczakDominik · 2019-09-06T19:44:54Z

plasmapy/atomic/parsing.py

+#        keyword_string = ", " + str(kwargs).strip(r"}{'").replace("'", "").replace(":", " =")
+#    else:
+#        keyword_string = ""
+#    return f"Particle({repr(arg)}{keyword_string})"


Dump the commented out code, maybe?

Oops! I forgot about this. Thanks!

StanczakDominik · 2019-09-06T19:45:56Z

plasmapy/atomic/tests/test_parsing.py

@@ -9,7 +9,7 @@
    _case_insensitive_aliases,
    _case_sensitive_aliases,
    _parse_and_check_atomic_input,
-    _call_string,
+    _particle_call_string,  # duplicate with utils.pytest_helpers.error_messages.call_string?


What's that about?

I made a more general plasampy.utils.pytest_helpers.call_string function which creates a string that replicates the call from a supplied callable, args, and kwargs to help with error messages, so this private function for a specific case is no longer necessary. I might as well go back and finish this change that I started but did not finish.

StanczakDominik · 2019-09-06T19:47:16Z

plasmapy/physics/__init__.py

@@ -36,6 +36,4 @@

 from .relativity import Lorentz_factor

-from plasmapy import transport
-


... wow, I have no idea why this was here. I'm surprised things didn't break by removing this!

namurphy · 2019-09-06T21:30:08Z

I saw pytest_helpers messing up when plugged into the parallel testing of pytest_xdist somewhere along the last few of my PRs

Huh, strange! If you have an idea for a failing test, that would be great. Otherwise I can try to play around with it to see what problems I might run into.

as this code base grows, we're going to be finding more edge cases... I guess my point is that we should really make sure we aren't reinventing Pytest's own functionalities in here and stick with the "canonical" solutions to problems.

My thought for the time being is that pytest_helpers should be for the testing patterns that come up over and over again for us, but are more specialized than what's in pytest. I don't think that pytest_helpers should deal with edge cases too much, as those cases won't correspond to a common testing pattern. I spent some time yesterday looking through pytest's documentation to get a better idea of what functionality already exists. I remember you bringing this up before, so I'm trying to keep this in mind as I'm working on this. There are also tools in Astropy too that we should make sure not to reinvent.

I want something like

>>> run_test(callable_object, args_list, kwargs_dict, expected_outcome)

that works if expected_outcome is a warning, exception, Quantity, Unit, number, ndarray, or whatever, and generates really useful error messages for all of these different types of comparisons. The canonical pytest patterns would be under the hood, so this would essentially be an interface so that we wouldn't have to write the same code over and over. I think this would simplify writing and reading for a lot of our tests. (Plus, having an interface like run_test would make it easier to change to a different testing framework besides pytest if we end up having strong reasons to, countless eons into the future.)

Move pytest_helpers & tests into own subdirectory

83596bb

namurphy added docs PlasmaPy Docs at http://docs.plasmapy.org status: ready for review PRs that are ready for code review testing breaking change For breaking changes, excluding deprecations and planned removals labels Sep 5, 2019

namurphy force-pushed the test-helpers-refactor branch from 2dfea2e to d103a51 Compare September 5, 2019 23:59

namurphy force-pushed the test-helpers-refactor branch from d103a51 to a2740fc Compare September 6, 2019 00:00

namurphy requested a review from StanczakDominik September 6, 2019 17:06

StanczakDominik approved these changes Sep 6, 2019

View reviewed changes

namurphy added this to the v0.3.0 milestone Sep 7, 2019

StanczakDominik merged commit d6a438d into PlasmaPy:master Sep 8, 2019

namurphy removed the status: ready for review PRs that are ready for code review label Oct 19, 2019

namurphy mentioned this pull request Dec 5, 2019

Refactor test helpers #728

Closed

namurphy deleted the test-helpers-refactor branch March 16, 2022 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pytest helpers to subdirectory of utils #674

Move pytest helpers to subdirectory of utils #674

namurphy commented Sep 5, 2019

pep8speaks commented Sep 5, 2019 •

edited

Loading

codecov bot commented Sep 5, 2019 •

edited

Loading

StanczakDominik left a comment

StanczakDominik Sep 6, 2019

namurphy Sep 6, 2019

StanczakDominik Sep 6, 2019

namurphy Sep 6, 2019

StanczakDominik Sep 6, 2019

namurphy commented Sep 6, 2019

		@@ -36,6 +36,4 @@

		from .relativity import Lorentz_factor

		from plasmapy import transport

Move pytest helpers to subdirectory of utils #674

Move pytest helpers to subdirectory of utils #674

Conversation

namurphy commented Sep 5, 2019

pep8speaks commented Sep 5, 2019 • edited Loading

Comment last updated at 2019-09-06 00:00:51 UTC

codecov bot commented Sep 5, 2019 • edited Loading

Codecov Report

StanczakDominik left a comment

Choose a reason for hiding this comment

StanczakDominik Sep 6, 2019

Choose a reason for hiding this comment

namurphy Sep 6, 2019

Choose a reason for hiding this comment

StanczakDominik Sep 6, 2019

Choose a reason for hiding this comment

namurphy Sep 6, 2019

Choose a reason for hiding this comment

StanczakDominik Sep 6, 2019

Choose a reason for hiding this comment

namurphy commented Sep 6, 2019

pep8speaks commented Sep 5, 2019 •

edited

Loading

codecov bot commented Sep 5, 2019 •

edited

Loading