Skip to content

Refactor test_scale_to function - continue to establish good practices - #255

Merged
sbillinge merged 8 commits into
diffpy:mainfrom
bobleesj:pytest-warning
Dec 20, 2024
Merged

Refactor test_scale_to function - continue to establish good practices#255
sbillinge merged 8 commits into
diffpy:mainfrom
bobleesj:pytest-warning

Conversation

@bobleesj

@bobleesjbobleesj commented Dec 19, 2024

Copy link
Copy Markdown
Contributor

The goal of this PR is to demonstrate how to refactor a specific test function.

  • Use variable names that are concise, clear, yet does not lose info
  • Correctly use @pytest.mark.parametrize to init objects
  • Differentiate info vs warning messages for end-users

Comment threadtests/test_diffraction_objects.py
@codecov

codecovBot commented Dec 19, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2bc765f) to head (206d39d).
Report is 12 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #255 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 8 8 Lines 380 378 -2 =========================================
- Hits 380 378 -2 
Files with missing linesCoverage Δ
tests/test_diffraction_objects.py100.00% <100.00%> (ø)

Comment threadtests/test_diffraction_objects.py
Comment threadtests/test_diffraction_objects.py
@bobleesj

Copy link
Copy Markdown
ContributorAuthor

@sbillinge ready for review - @yucongalicechen continuing to refactor test functions for future maintenance.

@sbillingesbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks great....but please see inline.

Comment threadsrc/diffpy/utils/transforms.py Outdated
inf_output_wmsg = (
"INFO: The largest output value in the array is infinite. This is allowed, but it will not be plotted."
)
inf_output_wmsg = "The largest output value in the array is infinite. This is allowed, but it will not be plotted."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is supposed to be an info not a warning. I think we want INFO and change wmsg to imsg in the name. Make sure it is a print and not a warning.warn in the code.

The group standard could be that a warning means that something is wrong, albeit not error generating.

Here it is "expected" that we will have infinities sometimes and we want to handle them if possible. So nothing is wrong but we want the user to alerted so they understand any magic.

Not sure if that all makes sense.

Comment threadtests/test_diffraction_objects.py
Comment threadtests/test_diffraction_objects.py
Comment threadtests/test_diffraction_objects.py
Comment threadtests/test_diffraction_objects.py Outdated
},
False,
),
( # One without wavelnegth, expect inequality

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

quick note to myself - fix typo

@sbillinge

Copy link
Copy Markdown
Contributor

@bobleesj LGTM. I am happy to merge whenever you are ready. Great improvement. Not sure if you did the high level scoping sttement. I have to board my flight though....

@bobleesj

Copy link
Copy Markdown
ContributorAuthor

@bobleesj LGTM. I am happy to merge whenever you are ready. Great improvement. Not sure if you did the high level scoping sttement. I have to board my flight though....

@sbillinge I will take some time to internalize the comments and make new commits later today. Thanks a lot! Safe travels to Africa.

@bobleesj

bobleesj commented Dec 20, 2024

Copy link
Copy Markdown
ContributorAuthor

@sbillinge Ready for review - the last commit is just quick fix from UC to Case as noted and adding "No news added".

For higher-level ones, I created separate issues: 1) how to enforce BG members to define testing scope before writing one 2) grouping test cases 3) review warning/info/error runtime error messages

@sbillinge
sbillinge merged commit 8eaf5d7 into diffpy:mainDec 20, 2024
@bobleesj
bobleesj deleted the pytest-warning branch December 20, 2024 23:36
@bobleesj

Copy link
Copy Markdown
ContributorAuthor

Noted the additional commits made with higher-level test comments.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@bobleesj@sbillinge