Continue refactor test functions, replace `UC` to `C` in `test_transforms.py` #266

bobleesj · 2024-12-22T02:24:05Z

No description provided.

codecov · 2024-12-22T02:25:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ce6c9cc) to head (17c87f0).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #266   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          404       402    -2     
=========================================
- Hits           404       402    -2

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

bobleesj · 2024-12-22T02:26:10Z

tests/test_diffraction_objects.py

In the group, do we want to call it "C" instead of "Case"?

I think either can work

bobleesj · 2024-12-22T02:28:46Z

tests/test_transforms.py

        (
            None,
            np.array([0, 30, 60, 90, 120, 180]),
            np.array([0, 1, 2, 3, 4, 5]),
        ),
-        # UC3: user specified valid tth values between 0-180 degrees (with wavelength)
+        # C3: valid tth values between 0-180 degrees (with wavelength)


I don't think we want to call it UC right? Since this is testing actual_q = tth_to_q(tth, wavelength)

We basically never want to see UCs in tests. UCs are enumerated lists of actions that describe user behavior when using an app.

bobleesj · 2024-12-22T02:29:26Z

tests/test_transforms.py

@@ -94,14 +96,14 @@ def test_tth_to_q(wavelength, tth, expected_q, wavelength_warning_msg):
 @pytest.mark.parametrize(
    "wavelength, tth, expected_error_type, expected_error_msg",
    [
-        # UC0: user specified an invalid tth value of > 180 degrees (without wavelength)
+        # C1: invalid tth value of > 180 degrees, no wavelength, expect two theta ValueError


same comment as above - testing tth_to_q(tth, wavelength)

Right, no UC

bobleesj · 2024-12-22T02:29:50Z

tests/test_transforms.py

@@ -171,13 +173,13 @@ def test_d_to_q(d, expected_q, zero_divide_error_expected):
    "wavelength, tth, expected_d, divide_by_zero_warning_expected",
    [
        # Test conversion of q to d with valid values
-        # Case 1: empty tth values, no, expect empty d values
+        # C1: empty tth values, no, expect empty d values


same comment - testing tth_to_d

Either is fine

tests/test_diffraction_objects.py

bobleesj · 2024-12-22T03:37:21Z

tests/test_diffraction_objects.py

        )


-params_index = [


just moving params_index under @pytest.mark.parametrize

bobleesj · 2024-12-22T03:37:49Z

tests/test_diffraction_objects.py

@@ -406,7 +411,7 @@ def test_dump(tmp_path, mocker):
 @pytest.mark.parametrize(
    "do_init_args, expected_do_dict, divide_by_zero_warning_expected",
    [
-        (  # instantiate just array attributes
+        (  # Instantiate just array attributes


Quick PEP8 standard, starting each comment with a cap letter

Yes, let's use that convention. For the test cases I don't mind if they are full sentences or not tbh as long as the intent is clear.

bobleesj · 2024-12-22T03:40:13Z

tests/test_transforms.py

-        # 2. Empty q values, wavelength specified, return empty arrays
+        # Test conversion of q to tth with valid values
+        # C1: Allow empty array q to compute tth with or without wavelength
+        # 1. Wavelength provided, expect empty array of tth


I think the word expect is better than return since expect can be used for warnings too.

Would you have any further feedback on this refactored version?

The flow is like..

"The overall goal is toTest conversion of q to tth with valid values. For the first case, we want to see if it Allows empty array q to compute tth with or without wavelength. Then, for the first case of Wavelength provided, expect empty array of tth. For the second case of No wavelength provided, still expect an empty tth but hey you will also get a warning!"

Your version looks great here
Very clear

…utils into runtime-warning

tests/test_transforms.py

bobleesj · 2024-12-22T03:47:54Z

@sbillinge ready for review

sbillinge

Please see inline. It looks great. I will go ahead and merge.

sbillinge · 2024-12-22T04:14:34Z

tests/test_diffraction_objects.py

+@pytest.mark.parametrize(
+    "org_do_args, target_do_args, scale_inputs",
+    [
+        # UC1: User did not specify anything


UC -> C. Also these comments don't seem to correspond to the tests. Not sure what is going on.

Wait, I read above... You will do these on separate PRs, sorry!

sbillinge · 2024-12-22T11:23:54Z

tests/test_transforms.py

        (
            None,
            np.array([0, 30, 60, 90, 120, 180]),
            np.array([0, 1, 2, 3, 4, 5]),
        ),
-        # UC3: user specified valid tth values between 0-180 degrees (with wavelength)
+        # C3: valid tth values between 0-180 degrees (with wavelength)


We basically never want to see UCs in tests. UCs are enumerated lists of actions that describe user behavior when using an app.

sbillinge · 2024-12-22T11:24:34Z

tests/test_transforms.py

@@ -94,14 +96,14 @@ def test_tth_to_q(wavelength, tth, expected_q, wavelength_warning_msg):
 @pytest.mark.parametrize(
    "wavelength, tth, expected_error_type, expected_error_msg",
    [
-        # UC0: user specified an invalid tth value of > 180 degrees (without wavelength)
+        # C1: invalid tth value of > 180 degrees, no wavelength, expect two theta ValueError


Right, no UC

sbillinge · 2024-12-22T11:24:51Z

tests/test_transforms.py

@@ -171,13 +173,13 @@ def test_d_to_q(d, expected_q, zero_divide_error_expected):
    "wavelength, tth, expected_d, divide_by_zero_warning_expected",
    [
        # Test conversion of q to d with valid values
-        # Case 1: empty tth values, no, expect empty d values
+        # C1: empty tth values, no, expect empty d values


Either is fine

sbillinge · 2024-12-22T11:26:04Z

tests/test_diffraction_objects.py

@@ -406,7 +411,7 @@ def test_dump(tmp_path, mocker):
 @pytest.mark.parametrize(
    "do_init_args, expected_do_dict, divide_by_zero_warning_expected",
    [
-        (  # instantiate just array attributes
+        (  # Instantiate just array attributes


Yes, let's use that convention. For the test cases I don't mind if they are full sentences or not tbh as long as the intent is clear.

sbillinge · 2024-12-22T11:27:00Z

tests/test_transforms.py

-        # 2. Empty q values, wavelength specified, return empty arrays
+        # Test conversion of q to tth with valid values
+        # C1: Allow empty array q to compute tth with or without wavelength
+        # 1. Wavelength provided, expect empty array of tth


Your version looks great here
Very clear

bobleesj · 2024-12-22T13:29:51Z

Yes, let's use that convention. For the test cases I don't mind if they are full sentences or not tbh as long as the intent is clear.

Right, method docstring also doesn't have to start with a complete "noun-verb" sentences anyway.. so i agree.

bobleesj and others added 3 commits December 21, 2024 21:22

Continue refactor test functions, replace UC to C

2dfb953

Add news containing no news

65d9d61

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

edd44ee

bobleesj commented Dec 22, 2024

View reviewed changes

tests/test_diffraction_objects.py Outdated Show resolved Hide resolved

bobleesj and others added 7 commits December 21, 2024 21:33

Fix minor typo from C. to C:

47750a0

Start comment with a cap letter

b61eaca

Refactor test_q_to_tth with full sentence

6570061

Refine test comment using Expect

df0f539

Another round of test comment fix

9188171

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

704ac0f

Continue to refactor test_tth_to_q_bad

d5e2cfd

bobleesj commented Dec 22, 2024

View reviewed changes

bobleesj added 2 commits December 21, 2024 22:40

Merge branch 'runtime-warning' of https://github.com/bobleesj/diffpy.…

c8d7a1f

…utils into runtime-warning

Add warning to one of the comments

17c87f0

bobleesj commented Dec 22, 2024

View reviewed changes

tests/test_transforms.py Outdated Show resolved Hide resolved

sbillinge reviewed Dec 22, 2024

View reviewed changes

sbillinge merged commit c67fec3 into diffpy:main Dec 22, 2024
5 checks passed

bobleesj deleted the runtime-warning branch December 22, 2024 13:30

		)


		params_index = [

Continue refactor test functions, replace UC to C in test_transforms.py #266

Continue refactor test functions, replace UC to C in test_transforms.py #266

Uh oh!

Conversation

bobleesj commented Dec 22, 2024

Uh oh!

codecov bot commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobleesj Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobleesj commented Dec 22, 2024

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobleesj commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Continue refactor test functions, replace `UC` to `C` in `test_transforms.py` #266

Continue refactor test functions, replace `UC` to `C` in `test_transforms.py` #266

codecov bot commented Dec 22, 2024 •

edited

Loading

bobleesj Dec 22, 2024 •

edited

Loading

bobleesj commented Dec 22, 2024 •

edited

Loading