Refactor `test_transfor.py`: pass variables to pytest parametrize, use `conftest.py` for common error messages #236

bobleesj · 2024-12-15T05:18:33Z

For non-UC behavior, we've decided to use the following format in the group as the group standard:

@pytest.mark.parametrize(
    "wavelength, q, expected_tth",
    [
        # Case 1: Allow empty arrays for q
        # 1. Empty q values, no wavelength, return empty arrays
        (None, np.empty((0)), np.empty((0))),
        # 2. Empty q values, wavelength specified, return empty arrays
        (4 * np.pi, np.empty((0)), np.empty(0)),
       
        # Case 2: Allow wavelength to be missing.
        # Valid q values, no wavelength, return index array
        (
            None,
            np.array([0, 0.2, 0.4, 0.6, 0.8, 1]),
            np.array([0, 1, 2, 3, 4, 5]),
        ),

        # Case 3: Correctly specified q and wavelength
        # Expected tth values are 2*arcsin(q) in degrees
        (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])),
    ],
)

…n the file

github-actions · 2024-12-15T05:19:02Z

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

codecov · 2024-12-15T05:20:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d919fef) to head (d962c1b).
Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #236   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          362       372   +10     
=========================================
+ Hits           362       372   +10

Files with missing lines	Coverage Δ
tests/conftest.py	`100.00% <100.00%> (ø)`
tests/test_transforms.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

bobleesj · 2024-12-15T05:21:49Z

tests/conftest.py

@@ -36,3 +36,21 @@ def _load(filename):
 @pytest.fixture
 def do_minimal_tth():
    return DiffractionObject(wavelength=2 * np.pi, xarray=np.array([30, 60]), yarray=np.array([1, 2]), xtype="tth")
+
+
+@pytest.fixture


The following two warning messages are used often in test_transform.py We could have access to this string within tests via conftest? We aren't really importing from src... so I think it really increases readability for some test cases (not all):

For example

From:

params_d_to_tth_bad = [ # UC1: user specified invalid d values that result in tth > 180 degrees ( [4 * np.pi, np.array([1.2, 1, 0.8, 0.6, 0.4, 0.2])], [ ValueError, "The supplied input array and wavelength will result in an impossible two-theta. " "Please check these values and re-instantiate the DiffractionObject with correct values.", ], ), # UC2: user specified a wrong wavelength that result in tth > 180 degrees ( [100, np.array([1, 0.8, 0.6, 0.4, 0.2, 0])], [ ValueError, "The supplied input array and wavelength will result in an impossible two-theta. " "Please check these values and re-instantiate the DiffractionObject with correct values.", ], ), ] @pytest.mark.parametrize("inputs, expected", params_d_to_tth_bad) def test_d_to_tth_bad(inputs, expected): with pytest.raises(expected[0], match=expected[1]): d_to_tth(inputs[1], inputs[0])

to:

@pytest.mark.parametrize( "wavelength, d, expected_error_type", [ # UC1: user specified invalid d values that result in tth > 180 degrees (4 * np.pi, np.array([1.2, 1, 0.8, 0.6, 0.4, 0.2]), ValueError), # UC2: user specified a wrong wavelength that result in tth > 180 degrees (100, np.array([1, 0.8, 0.6, 0.4, 0.2, 0]), ValueError), ], ) def test_d_to_tth_bad(wavelength, d, expected_error_type, invalid_q_or_d_or_wavelength_error_msg): expected_error_msg = invalid_q_or_d_or_wavelength_error_msg with pytest.raises(expected_error_type, match=expected_error_msg): d_to_tth(d, wavelength)

Yes on that

bobleesj · 2024-12-15T05:26:32Z

tests/test_transforms.py

No numbers have been modified.

Here primarily removed extra variables such as params_q_to_tth. First thought was that, it would be best to not have variables shared globally at the file level.

Second thought was that

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # UC1: Empty q values, no wavelength, return empty arrays (None, np.empty((0)), np.empty((0))),

Feels more natural - the first two lines are like - "hey these are testing variables that we provide. The the last parameter is what we expect. Of course, if you are interested... take a look at the rest of the lines for specific UCs."

Yes, I agree. In future we would like to spend 75% of thinking/planning/reviewing time on the tests, then 25% on the code target than vice versa and so the more the tests are readable and interpretable and syntactically codified the better. This dress well, as you describe with an extra comment that captures the behavioral intent.

Even better for the behavior would be something like. "Minimally support empty arrays"

Or both comments, behavior and example of behavior

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # UC1.1: User specified empty 'q' and no 'wavelength'. # Expect empty 'tth' array and UserWarning about missing wavelength. (None, np.empty((0)), np.empty((0))), # UC1.2: User specified empty 'q' and 'wavelength'. Expect empty 'tth' array. (4 * np.pi, np.empty((0)), np.empty(0)), # UC2.1: User specified non-empty 'q' values and no 'wavelength'. # Expect non-empty 'tth' array and UserWarning about missing wavelength. ( None, np.array([0, 0.2, 0.4, 0.6, 0.8, 1]), np.array([0, 1, 2, 3, 4, 5]), ), # UC2.2: User specified non-empty 'q' values and 'wavelength'. # Expect tth values are 2*arcsin(q) in degrees. (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])), ], )

This is the change made since the last commit - it's hard to read below, so pasting the change here. How do you like it? I've used UC1.1 and UC1.2 (w/o q) vs. UC2.1 and UC2.2 (with q).

Also having a blank line between each UC feels a bit more readable?

@sbillinge ready for review - this is the only change made since your last review. (It's getting a bit big..)

this is not quite what I had in mind. I think it was good before, but just needed a kind of overriding framing. Also, this is not the correct use of UCs per how we do it in the group, so I don't want to propagate wrong ideas by calling these UCs.

Maybe it is easier if I put what I had in mind below

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # Case 1: Allow empty arrays # 1. Empty q values, no wavelength, return empty arrays (None, np.empty((0)), np.empty((0))), # 2: Empty q values, wavelength specified, return empty arrays (4 * np.pi, np.empty((0)), np.empty(0)), # Case 2: allow wavelength to be missing. # 3: user specified valid q values, no wavelength, return index array ( None, np.array([0, 0.2, 0.4, 0.6, 0.8, 1]), np.array([0, 1, 2, 3, 4, 5]), ), # Case 3: correctly specified q and wavelength # expected tth values are 2*arcsin(q) in degrees (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])), ], )

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # Case 1: Allow empty arrays for q # 1. Empty q values, no wavelength, return empty arrays (None, np.empty((0)), np.empty((0))), # 2. Empty q values, wavelength specified, return empty arrays (4 * np.pi, np.empty((0)), np.empty(0)), # Case 2: Allow wavelength to be missing. # Valid q values, no wavelength, return index array ( None, np.array([0, 0.2, 0.4, 0.6, 0.8, 1]), np.array([0, 1, 2, 3, 4, 5]), ), # Case 3: Correctly specified q and wavelength # Expected tth values are 2*arcsin(q) in degrees (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])), ], )

Thank you @sbillinge. Above is used for one of the refactored test functions. For other test functions, I could further make improvements via new PRs. Adding more here is a bit hard for you and me to track things..

bobleesj · 2024-12-15T05:27:55Z

tests/test_transforms.py

    assert np.allclose(actual_q, expected_q)


-params_tth_to_q_bad = [


Lots of red lines above/below, but again, no manual fixes have been done..

bobleesj

Ready for review! @sbillinge and cc'ing @yucongalicechen for your second look!

sbillinge

Yes, I like this very much. Two thumbs up. Please see my inline comments, but let's use this as our own kind of testing PEP.

For the behavior/example we could group then with s blank line of there are a few different rxamples of the same behavior.

sbillinge · 2024-12-15T07:26:22Z

tests/conftest.py

@@ -36,3 +36,21 @@ def _load(filename):
 @pytest.fixture
 def do_minimal_tth():
    return DiffractionObject(wavelength=2 * np.pi, xarray=np.array([30, 60]), yarray=np.array([1, 2]), xtype="tth")
+
+
+@pytest.fixture


Yes on that

sbillinge · 2024-12-15T07:29:51Z

tests/test_transforms.py

Yes, I agree. In future we would like to spend 75% of thinking/planning/reviewing time on the tests, then 25% on the code target than vice versa and so the more the tests are readable and interpretable and syntactically codified the better. This dress well, as you describe with an extra comment that captures the behavioral intent.

sbillinge · 2024-12-15T07:31:09Z

tests/test_transforms.py

Even better for the behavior would be something like. "Minimally support empty arrays"

sbillinge · 2024-12-15T07:31:48Z

tests/test_transforms.py

Or both comments, behavior and example of behavior

bobleesj · 2024-12-15T21:51:29Z

@sbillinge ready for review - made a change shown in the latest commit below and replied to your in-line comments.

ef75197

sbillinge

please see reply to your comment

sbillinge · 2024-12-16T00:01:03Z

tests/test_transforms.py

this is not quite what I had in mind. I think it was good before, but just needed a kind of overriding framing. Also, this is not the correct use of UCs per how we do it in the group, so I don't want to propagate wrong ideas by calling these UCs.

Maybe it is easier if I put what I had in mind below

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # Case 1: Allow empty arrays # 1. Empty q values, no wavelength, return empty arrays (None, np.empty((0)), np.empty((0))), # 2: Empty q values, wavelength specified, return empty arrays (4 * np.pi, np.empty((0)), np.empty(0)), # Case 2: allow wavelength to be missing. # 3: user specified valid q values, no wavelength, return index array ( None, np.array([0, 0.2, 0.4, 0.6, 0.8, 1]), np.array([0, 1, 2, 3, 4, 5]), ), # Case 3: correctly specified q and wavelength # expected tth values are 2*arcsin(q) in degrees (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])), ], )

bobleesj

@sbillinge ready for review - replied to your in-line comment

bobleesj · 2024-12-16T02:45:35Z

tests/test_transforms.py

@pytest.mark.parametrize( "wavelength, q, expected_tth", [ # Case 1: Allow empty arrays for q # 1. Empty q values, no wavelength, return empty arrays (None, np.empty((0)), np.empty((0))), # 2. Empty q values, wavelength specified, return empty arrays (4 * np.pi, np.empty((0)), np.empty(0)), # Case 2: Allow wavelength to be missing. # Valid q values, no wavelength, return index array ( None, np.array([0, 0.2, 0.4, 0.6, 0.8, 1]), np.array([0, 1, 2, 3, 4, 5]), ), # Case 3: Correctly specified q and wavelength # Expected tth values are 2*arcsin(q) in degrees (4 * np.pi, np.array([0, 1 / np.sqrt(2), 1.0]), np.array([0, 90.0, 180.0])), ], )

Thank you @sbillinge. Above is used for one of the refactored test functions. For other test functions, I could further make improvements via new PRs. Adding more here is a bit hard for you and me to track things..

bobleesj added 4 commits December 14, 2024 23:45

Remove 2 wavelength warnings with test_tth_to_q func

5d88cf6

Refactor transform tests, pass variables to parameters

73d8ada

Remove the use of extra variable for params to prevent global scope i…

5e39538

…n the file

Apply pre-commit

cdf96df

bobleesj commented Dec 15, 2024

View reviewed changes

bobleesj marked this pull request as ready for review December 15, 2024 05:28

bobleesj changed the title ~~Refactor test_transfor.py: pass variables to functions, maintain function scope, use conftest.py for common error messages~~ Refactor test_transfor.py: pass variables to pytest parametrize, use conftest.py for common error messages Dec 15, 2024

bobleesj mentioned this pull request Dec 15, 2024

release: v3.6.0 issues overview #143

Closed

59 tasks

sbillinge reviewed Dec 15, 2024

View reviewed changes

bobleesj and others added 2 commits December 15, 2024 16:45

improve UC comment for test_q_to_tth func

ef75197

[pre-commit.ci] auto fixes from pre-commit hooks

8ebaede

sbillinge reviewed Dec 16, 2024

View reviewed changes

bobleesj and others added 2 commits December 15, 2024 21:38

Do not call UCs for test function

9343e86

[pre-commit.ci] auto fixes from pre-commit hooks

d962c1b

bobleesj commented Dec 16, 2024

View reviewed changes

sbillinge merged commit e6a006d into diffpy:main Dec 16, 2024
4 of 5 checks passed

bobleesj deleted the pytest-warnings branch December 16, 2024 14:30

bobleesj mentioned this pull request Dec 16, 2024

write tests for... #187

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `test_transfor.py`: pass variables to pytest parametrize, use `conftest.py` for common error messages #236

Refactor `test_transfor.py`: pass variables to pytest parametrize, use `conftest.py` for common error messages #236

bobleesj commented Dec 15, 2024 •

edited

Loading

github-actions bot commented Dec 15, 2024

codecov bot commented Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024

sbillinge Dec 15, 2024

bobleesj Dec 15, 2024 •

edited

Loading

sbillinge Dec 15, 2024

sbillinge Dec 15, 2024

sbillinge Dec 15, 2024

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024 •

edited

Loading

sbillinge Dec 16, 2024

bobleesj Dec 16, 2024

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj left a comment

sbillinge left a comment

sbillinge Dec 15, 2024

sbillinge Dec 15, 2024

sbillinge Dec 15, 2024

sbillinge Dec 15, 2024

bobleesj commented Dec 15, 2024 •

edited

Loading

sbillinge left a comment

sbillinge Dec 16, 2024

bobleesj left a comment

bobleesj Dec 16, 2024

		assert np.allclose(actual_q, expected_q)


		params_tth_to_q_bad = [

Refactor test_transfor.py: pass variables to pytest parametrize, use conftest.py for common error messages #236

Refactor test_transfor.py: pass variables to pytest parametrize, use conftest.py for common error messages #236

Conversation

bobleesj commented Dec 15, 2024 • edited Loading

github-actions bot commented Dec 15, 2024

codecov bot commented Dec 15, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

bobleesj Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj Dec 15, 2024 • edited Loading

Choose a reason for hiding this comment

bobleesj left a comment

Choose a reason for hiding this comment

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj commented Dec 15, 2024 • edited Loading

sbillinge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobleesj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `test_transfor.py`: pass variables to pytest parametrize, use `conftest.py` for common error messages #236

Refactor `test_transfor.py`: pass variables to pytest parametrize, use `conftest.py` for common error messages #236

bobleesj commented Dec 15, 2024 •

edited

Loading

codecov bot commented Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj Dec 15, 2024 •

edited

Loading

bobleesj commented Dec 15, 2024 •

edited

Loading