Test helper refactoring 2: Refactor and move functionality for generating call strings used in error messages #920

namurphy · 2020-10-12T01:11:03Z

The second spin-off PR from #726. This should be reviewed and merged after #919.

We currently have a function call_string which generates a string that can reproduce a particular call. This function is currently used in test helper functionality when generating error messages. Its potential uses go beyond creating error messages, though, such as in __repr__ methods. This PR moves that functionality to plasmapy.utils.formatting, and creates functions for reproducing the call strings of functions and attributes/methods of class instances.

This originally came from PlasmaPy#726, but was spun off into a different PR.

codecov · 2020-10-12T01:13:35Z

Codecov Report

Merging #920 (e5647a9) into master (95aa647) will increase coverage by 0.02%.
The diff coverage is 95.18%.

@@            Coverage Diff             @@
##           master     #920      +/-   ##
==========================================
+ Coverage   96.29%   96.31%   +0.02%     
==========================================
  Files          62       61       -1     
  Lines        5715     5341     -374     
==========================================
- Hits         5503     5144     -359     
+ Misses        212      197      -15

Impacted Files	Coverage Δ
plasmapy/utils/__init__.py	`100.00% <ø> (ø)`
plasmapy/utils/formatting/formatting.py	`94.11% <94.11%> (ø)`
plasmapy/plasma/sources/plasmablob.py	`100.00% <100.00%> (ø)`
plasmapy/utils/exceptions.py	`100.00% <100.00%> (ø)`
plasmapy/utils/formatting/__init__.py	`100.00% <100.00%> (ø)`
plasmapy/utils/pytest_helpers/pytest_helpers.py	`91.56% <100.00%> (-1.11%)`	⬇️
plasmapy/analysis/fit_functions.py
plasmapy/analysis/__init__.py
... and 1 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 5a2b99b...41f57cf. Read the comment docs.

namurphy · 2020-10-12T02:07:13Z

I removed colorama as a dependency, in part because it sometimes led to weird formatting in terminals.
I sorted imports with isort and reformatted with black for a small number of files elsewhere in the package.

…rs-formatting

I set this warning to not be included in coverage tests because it's likely that we'll have times when no functions have been deprecated.

…rs-formatting

plasmapy/utils/code_repr.py

rocco8773 · 2021-01-26T21:55:55Z

plasmapy/utils/tests/test_code_repr.py

+        (5.3 * u.m, "5.3*u.m"),
+        (5.4 / u.m, "5.4/u.m"),
+        (5.5 * u.m ** -2, "5.5*u.m**-2"),
+        (5.0 * u.dimensionless_unscaled, "5.0*u.dimensionless_unscaled"),
+        (np.array([3.5, 4.2]) * u.m ** -2.5, "np.array([3.5, 4.2])*u.m**-2.5"),


For completeness, maybe throw in a case that looks like 5.0 / u.m ** 3.

rocco8773 · 2021-01-26T22:00:41Z

plasmapy/utils/code_repr.py

+    function_for_type = {
+        u.Quantity: _code_repr_of_quantity,
+        np.ndarray: _code_repr_of_ndarray,
+    }


Should probably add _code_repr_...'s for long lists, tuples, and dicts. I'm guessing these cases will turn up quickly.

rocco8773 · 2021-01-26T22:01:28Z

plasmapy/utils/tests/test_code_repr.py

+        (1, "1"),
+        ("asdf", "'asdf'"),
+        (3.42, "3.42"),
+        ([3.42, 3.84], "[3.42, 3.84]"),
+        (4.2 * u.m, "4.2*u.m"),
+        (np.array([1, 2, 3]), "np.array([1, 2, 3])"),
+        (np.array([[1, 2], [3, 4]]), "np.array([[1, 2], [3, 4]])"),
+        (np.array([1.0, 2.0, 3.0]) * u.m / u.s, "np.array([1., 2., 3.])*u.m/u.s"),


How about test cases for lists, tuples, and dicts?

plasmapy/utils/code_repr.py

rocco8773 · 2021-01-26T22:19:20Z

plasmapy/utils/tests/test_code_repr.py

+    args_and_kwargs = _code_repr_of_args_and_kwargs(args, kwargs)
+    if args_and_kwargs != expected:
+        pytest.fail(
+            f"_code_repr_of_args_and_kwargs with the following arguments:\n"
+            f"  args:   {args}\n"
+            f"  kwargs: {kwargs}\n"
+            f"is not returning the expected string:\n"
+            f"  expected: {repr(expected)}\n"
+            f"  actual:   {repr(args_and_kwargs)}"
+        )


Testing and reporting failures this way seems like it's sidestepping the assert mechanics. Why not do something more like...

Suggested change

args_and_kwargs = _code_repr_of_args_and_kwargs(args, kwargs)

if args_and_kwargs != expected:

pytest.fail(

f"_code_repr_of_args_and_kwargs with the following arguments:\n"

f" args: {args}\n"

f" kwargs: {kwargs}\n"

f"is not returning the expected string:\n"

f" expected: {repr(expected)}\n"

f" actual: {repr(args_and_kwargs)}"

)

args_and_kwargs = _code_repr_of_args_and_kwargs(args, kwargs)

assert args_and_kwargs == expected, (

f"_code_repr_of_args_and_kwargs with the following arguments:\n"

f" args: {args}\n"

f" kwargs: {kwargs}\n"

f"is not returning the expected string:\n"

f" expected: {repr(expected)}\n"

)

I do think this approach should be applied to all similar cases.

plasmapy/utils/code_repr.py

rocco8773 · 2021-01-26T23:40:51Z

plasmapy/utils/tests/test_code_repr.py

+# function, args, kwargs, expected
+call_string_table = [
+    (generic_function, (), {}, "generic_function()"),
+    (generic_function, (1), {}, "generic_function(1)"),
+    (generic_function, ("x"), {}, "generic_function('x')"),
+    (generic_function, (1, "b", {}), {}, "generic_function(1, 'b', {})"),
+    (generic_function, (), {"kw": 1}, "generic_function(kw=1)"),
+    (generic_function, (), {"x": "c"}, "generic_function(x='c')"),
+    (
+        generic_function,
+        (1, 2, 3),
+        {"a": 4, "b": 5, "c": 6},
+        "generic_function(1, 2, 3, a=4, b=5, c=6)",
+    ),
+    (
+        generic_function,
+        np.array((1.0, 2.0)),
+        {},
+        "generic_function(np.array([1., 2.]))",
+    ),
+    (
+        generic_function,
+        np.array((3.0, 4.0)) * u.m,
+        {},
+        "generic_function(np.array([3., 4.])*u.m)",
+    ),
+    (
+        generic_function,
+        (ValueError,),
+        {"x": TypeError},
+        "generic_function(ValueError, x=TypeError)",
+    ),
+    (generic_function, (), {"bowl": super}, "generic_function(bowl=super)"),
+    (
+        generic_function,
+        (1, "b"),
+        {"b": 42, "R2": "D2"},
+        "generic_function(1, 'b', b=42, R2='D2')",
+    ),
+]
+
+
+@pytest.mark.parametrize("function,args,kwargs,expected", call_string_table)
+def test_call_string(function, args, kwargs, expected):


Unless call_string_table (and the like below) is (are) going to be called in several locations, I find that this disassociation of the parametrized list and the test function to significantly hinder readability of tests. I mean, I start reading the test and then I have to wonder away from the test to get the cases that are being tested.

I think the main problem with readability here is that the when we parametrize tests using a list containing a series of tuples which might contain inner tuples, the parameters get detached from the corresponding semantic information/metadata. It'd be so much more readable if the arguments were defined like keywords. In #925 I define classes that are intended to address this problem...without me having to rewrite essentially the same code over and over again. I might try doing something like that here, though independent of #925, since I'm trying to avoid using the test helpers to test the test helpers.

I agree with that sentiment. pytest.mark.parametrize's separation of names and values does make it a bit difficult to follow.

My point here is that when someone reads @pytest.mark.parametrize("function,args,kwargs,expected", call_string_table) they have to immediately search the code to find call_string_table. The fact that I have to leave the test to find out what is being tested is what reduces readability for me.

plasmapy/utils/code_repr.py

Co-authored-by: Erik Everson <[email protected]>

The docstring now says that these functions **approximate** the call string, which is easier to promise than providing an exact docstring.

- Use assert instead of pytest.fail - Define namedtuples to provide semantic information in parametrize

The name cls_args and cls_kwargs are potentially ambiguous, since it could mean that these are args associated with a class or a class instance. These are renamed to args_to_cls and kwargs_to_cls (and similarly for method_args and method_kwargs).

namurphy · 2021-01-27T23:47:52Z

Changes:

I renamed cls_args to args_to_cls (and same to similar arguments)
I created namedtuples to make it easier to understand the things in parametrize
- I thought...hey! This is a job for dataclasses! But it turns out, it wasn't. Keep it simple splendorously.
I added some tests with a list or tuple as the argument. I found a bug. I fixed it.
I made some optional arguments keyword-only in order to enforce
I added a parameter max_items that can limit the number of items that will get printed out in an ndarray or a Quantity
Otherwise, I adopted most @rocco8773's suggestions.

rocco8773

I'm satisfied. 😁 ... Looks good. The namedtuple is an interesting approach to the problem, would not have thought about it myself. I'm guessing you decided to leave the code_repr functions for lists, tuples, and dicts for a separate PR, correct?

rocco8773 · 2021-01-28T02:05:23Z

plasmapy/utils/tests/test_code_repr.py

+        quantity_case(quantity=5.5 * u.m ** -2, expected="5.5*u.m**-2"),
+        quantity_case(


Suggested change

quantity_case(quantity=5.5 * u.m ** -2, expected="5.5*u.m**-2"),

quantity_case(

quantity_case(quantity=5.5 * u.m ** -2, expected="5.5*u.m**-2"),

quantity_case(quantity=5.5 / u.m ** 2, expected="5.5*u.m**-2"),

quantity_case(

namurphy added 3 commits October 11, 2020 15:37

Initial commit for plasmapy.tests.helpers

12e06e8

Create plasmapy.utils.formatting

92ba287

This originally came from PlasmaPy#726, but was spun off into a different PR.

Update docstrings in formatting.py

316b83d

namurphy added testing plasmapy.utils Related to the plasmapy.utils subpackage labels Oct 12, 2020

namurphy added 6 commits October 11, 2020 21:15

Do not import contents of utils.formatting into utils

b8ac16b

Create api_static doc stub for utils.formatting.formatting

f4465f9

Include plasmapy.utils.formatting in utils narrative docs

10e5739

Update imports related to utils.formatting

e7aec61

Remove colorama as a dependency

1627768

Sort references and fix code formatting

a40700c

namurphy force-pushed the test-helpers-formatting branch from 9ecd89b to a40700c Compare October 12, 2020 01:54

Add changelog entries for PlasmaPy#920

5458c31

namurphy mentioned this pull request Oct 12, 2020

Discontinue importing functions, classes, & exceptions directly into plasmapy.utils #921

Closed

namurphy added 2 commits October 11, 2020 22:34

Try to fix pre-commit formatting issue

6ba7f57

Fix formatting issue in pre-commit

bb8ccc8

namurphy changed the title ~~Refactor and move functionality for generating call strings used in error messages~~ Test helper refactoring 2: Refactor and move functionality for generating call strings used in error messages Oct 12, 2020

This was referenced Oct 13, 2020

Tasks to perform after the test helper refactoring is complete #930

Closed

Refactor test helpers #728

Closed

Fix multi-line docstring formatting

ad76699

namurphy marked this pull request as draft October 15, 2020 22:47

namurphy added 5 commits October 22, 2020 21:20

Merge branch 'master' of github.com:PlasmaPy/PlasmaPy into test-helpe…

d1f5946

…rs-formatting

Test private function that strings together warnings & warning messages

dbd957e

Improve tests of utils.formatting

6b8d7a4

Use new PlasmaPyDeprecationWarning decorator on run_test

aff058a

I set this warning to not be included in coverage tests because it's likely that we'll have times when no functions have been deprecated.

Correct deprecation warning inheritance

9b377de

namurphy mentioned this pull request Oct 24, 2020

Create @deprecated decorator based off of Astropy's #945

Closed

Merge branch 'master' of github.com:PlasmaPy/PlasmaPy into test-helpe…

3faef3b

…rs-formatting

rocco8773 reviewed Jan 26, 2021

View reviewed changes