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

Improve test comments under @pytest.mark.parametrize in test_diffraction_objects.py #273

Merged
merged 8 commits into from
Dec 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/pytest-comment.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
**Added:**

* Group's Pytest practices for using @pytest.mark.parametrize in test_diffraction_objects.py

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>
141 changes: 110 additions & 31 deletions tests/test_diffraction_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@


@pytest.mark.parametrize(
"do_args_1, do_args_2, expected_equality, warning_expected",
"do_args_1, do_args_2, expected_equality, wavelength_warning_expected",
[
# Test when __eq__ returns True and False
# Identical args, expect equality
# C1: Identical args, expect equality
(
{
"name": "same",
Expand All @@ -38,7 +38,8 @@
True,
False,
),
( # Different names, expect inequality
# Different names, expect inequality
(
{
"name": "something",
"xtype": "tth",
Expand All @@ -56,7 +57,8 @@
False,
True,
),
( # One without wavelength, expect inequality
# C2: One without wavelength, expect inequality
(
{
"wavelength": 0.71,
"xtype": "tth",
Expand All @@ -73,7 +75,8 @@
False,
True,
),
( # Different wavelengths, expect inequality
# C3: Different wavelength values, expect inequality
(
{
"wavelength": 0.71,
"xtype": "tth",
Expand All @@ -91,7 +94,8 @@
False,
False,
),
( # Different scat_quantity, expect inequality
# C4: Different scat_quantity, expect inequality
(
{
"scat_quantity": "x-ray",
"xtype": "tth",
Expand All @@ -109,7 +113,8 @@
False,
True,
),
( # Different q xarray values, expect inequality
# C5: Different q xarray values, expect inequality
(
{
"xtype": "q",
"xarray": np.array([1.0, 2.0]),
Expand All @@ -124,7 +129,8 @@
False,
True,
),
( # Different metadata, expect inequality
# C6: Different metadata, expect inequality
(
{
"xtype": "q",
"xarray": np.empty(0),
Expand All @@ -143,9 +149,9 @@
],
)
def test_diffraction_objects_equality(
do_args_1, do_args_2, expected_equality, warning_expected, wavelength_warning_msg
do_args_1, do_args_2, expected_equality, wavelength_warning_expected, wavelength_warning_msg
):
if warning_expected:
if wavelength_warning_expected:
with pytest.warns(UserWarning, match=re.escape(wavelength_warning_msg)):
do_1 = DiffractionObject(**do_args_1)
do_2 = DiffractionObject(**do_args_2)
Expand All @@ -158,9 +164,15 @@ def test_diffraction_objects_equality(
@pytest.mark.parametrize(
"xtype, expected_xarray",
[
# Test whether on_xtype returns the correct xarray values.
# C1: tth to tth, expect no change in xarray value
# 1. "tth" provided, expect tth
# 2. "2theta" provided, expect tth
("tth", np.array([30, 60])),
("2theta", np.array([30, 60])),
# C2: "q" provided, expect q converted from tth
("q", np.array([0.51764, 1])),
# C3: "d" provided, expect d converted from tth
("d", np.array([12.13818, 6.28319])),
],
)
Expand All @@ -185,8 +197,8 @@ def test_init_invalid_xtype():
@pytest.mark.parametrize(
"org_do_args, target_do_args, scale_inputs, expected",
[
# Test that scale_to() scales to the correct values
# C1: Same x-array and y-array, check offset
# Test whether scale_to() scales to the expected values
# C1: Same x-array and y-array with 2.1 offset, expect yarray shifted by 2.1 offset
(
{
"xarray": np.array([10, 15, 25, 30, 60, 140]),
Expand Down Expand Up @@ -290,7 +302,8 @@ def test_scale_to(org_do_args, target_do_args, scale_inputs, expected):
@pytest.mark.parametrize(
"org_do_args, target_do_args, scale_inputs",
[
# UC1: User did not specify anything
# Test expected errors produced from scale_to() with invalid inputs
# C1: none of q, tth, d, provided, expect ValueError
(
{
"xarray": np.array([0.1, 0.2, 0.3]),
Expand All @@ -311,7 +324,7 @@ def test_scale_to(org_do_args, target_do_args, scale_inputs, expected):
"offset": 0,
},
),
# UC2: User specified more than one of q, tth, and d
# C2: tth and d both provided, expect ValueErrort
(
{
"xarray": np.array([10, 25, 30.1, 40.2, 61, 120, 140]),
Expand Down Expand Up @@ -350,21 +363,84 @@ def test_scale_to_bad(org_do_args, target_do_args, scale_inputs):


@pytest.mark.parametrize(
"wavelength, xarray, yarray, xtype_1, xtype_2, value, expected_index",
"do_args, get_array_index_inputs, expected_index",
[
# UC1: Exact match
(4 * np.pi, np.array([30.005, 60]), np.array([1, 2]), "tth", "tth", 30.005, [0]),
# UC2: Target value lies in the array, returns the (first) closest index
(4 * np.pi, np.array([30, 60]), np.array([1, 2]), "tth", "tth", 45, [0]),
(4 * np.pi, np.array([30, 60]), np.array([1, 2]), "tth", "q", 0.25, [0]),
# UC3: Target value out of the range, returns the closest index
(4 * np.pi, np.array([0.25, 0.5, 0.71]), np.array([1, 2, 3]), "q", "q", 0.1, [0]),
(4 * np.pi, np.array([30, 60]), np.array([1, 2]), "tth", "tth", 63, [1]),
# Test get_array_index() returns the expected index given xtype and value
# C1: Target value is in the xarray and xtype is identical, expect exact index match
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not modify this PR again, but I do wonder whether it would be more compact as

        (  # C1: Target value is in the xarray and xtype is identical, expect exact index match
            {
                "wavelength": 4 * np.pi,
                "xarray": np.array([30.005, 60]),
                "yarray": np.array([1, 2]),
                "xtype": "tth",
            },
            {
                "xtype": "tth",
                "value": 30.005,
            },
            [0],
        ),

than the current

    # C1: Target value is in the xarray and xtype is identical, expect exact index match
        (
            {
                "wavelength": 4 * np.pi,
                "xarray": np.array([30.005, 60]),
                "yarray": np.array([1, 2]),
                "xtype": "tth",
            },
            {
                "xtype": "tth",
                "value": 30.005,
            },
            [0],
        ),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-12-25 at 10 07 41 AM

@sbillinge yeah I like that! The automatic indentation also provides a good visual hierarchy!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-12-25 at 10 11 26 AM

For single-lined cases, I think we still want to maintain this format right?

(
{
"wavelength": 4 * np.pi,
"xarray": np.array([30.005, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "tth",
"value": 30.005,
},
[0],
),
# C2: Target value lies in the array, expect the (first) closest index
(
{
"wavelength": 4 * np.pi,
"xarray": np.array([30, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "tth",
"value": 45,
},
[0],
),
(
{
"wavelength": 4 * np.pi,
"xarray": np.array([30, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "q",
"value": 0.25,
},
[0],
),
# C3: Target value out of the range, expect the closest index
# 1. Test with xtype of "q"
(
{
"wavelength": 4 * np.pi,
"xarray": np.array([0.25, 0.5, 0.71]),
"yarray": np.array([1, 2, 3]),
"xtype": "q",
},
{
"xtype": "q",
"value": 0.1,
},
[0],
),
# 2. Test with xtype of "tth"
(
{
"wavelength": 4 * np.pi,
"xarray": np.array([30, 60]),
"yarray": np.array([1, 2]),
"xtype": "tth",
},
{
"xtype": "tth",
"value": 63,
},
[1],
),
],
)
def test_get_array_index(wavelength, xarray, yarray, xtype_1, xtype_2, value, expected_index):
do = DiffractionObject(wavelength=wavelength, xarray=xarray, yarray=yarray, xtype=xtype_1)
actual_index = do.get_array_index(value=value, xtype=xtype_2)
def test_get_array_index(do_args, get_array_index_inputs, expected_index):
do = DiffractionObject(**do_args)
actual_index = do.get_array_index(get_array_index_inputs["value"], get_array_index_inputs["xtype"])
assert actual_index == expected_index


Expand Down Expand Up @@ -411,7 +487,9 @@ def test_dump(tmp_path, mocker):
@pytest.mark.parametrize(
"do_init_args, expected_do_dict, divide_by_zero_warning_expected",
[
( # Instantiate just array attributes
# Test __dict__ of DiffractionObject instance initialized with valid arguments
(
# C1: Minimum arguments provided for init, expect all attributes set without None
{
"xarray": np.array([0.0, 90.0, 180.0]),
"yarray": np.array([1.0, 2.0, 3.0]),
Expand Down Expand Up @@ -440,7 +518,8 @@ def test_dump(tmp_path, mocker):
},
True,
),
( # Instantiate just array attributes
# C2: Initialize with an optional scat_quantity argument, expect non-empty string for scat_quantity
(
{
"xarray": np.array([np.inf, 2 * np.sqrt(2) * np.pi, 2 * np.pi]),
"yarray": np.array([1.0, 2.0, 3.0]),
Expand Down Expand Up @@ -487,11 +566,12 @@ def test_init_valid(do_init_args, expected_do_dict, divide_by_zero_warning_expec
@pytest.mark.parametrize(
"do_init_args, expected_error_msg",
[
( # C1: No arguments provided
# Test expected error messages when 3 required arguments not provided in DiffractionObject init
( # C1: No arguments provided, expect 3 required positional arguments error
{},
"missing 3 required positional arguments: 'xarray', 'yarray', and 'xtype'",
),
( # C2: Only xarray and yarray provided
( # C2: Only xarray and yarray provided, expect 1 required positional argument error
{"xarray": np.array([0.0, 90.0]), "yarray": np.array([0.0, 90.0])},
"missing 1 required positional argument: 'xtype'",
),
Expand All @@ -514,7 +594,6 @@ def test_all_array_getter(do_minimal_tth):

def test_all_array_setter(do_minimal):
do = do_minimal
# Attempt to directly modify the property
with pytest.raises(
AttributeError,
match="Direct modification of attribute 'all_arrays' is not allowed. "
Expand Down
Loading