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

testing code cleanup #449

Merged
merged 7 commits into from
Apr 30, 2024
Merged

testing code cleanup #449

merged 7 commits into from
Apr 30, 2024

Conversation

jGaboardi
Copy link
Member

@jGaboardi jGaboardi commented Apr 27, 2024

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.8%. Comparing base (785b396) to head (9707567).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #449   +/-   ##
=====================================
  Coverage   77.8%   77.8%           
=====================================
  Files         27      27           
  Lines       2638    2638           
=====================================
  Hits        2053    2053           
  Misses       585     585           
Files Coverage Δ
spopt/locate/util.py 100.0% <100.0%> (ø)

Copy link
Member

@gegen07 gegen07 left a comment

Choose a reason for hiding this comment

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

Excellent work @jGaboardi! The PR smoothens the code a bit and adds more reusability.

  • I noted some warnings were not moved to conftest.py. Is there a specific reason for deciding which test should use the conftest.py warning?
  • About the numpy deprecationWarning, I think we should filter it, as it seems, it will disappear in future releases of pyproj.

Copy link
Member

Choose a reason for hiding this comment

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

There is a similar infeasibility warning in line 86. Should we move all expected warnings to conftest.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a similar infeasibility warning in line 86. Should we move all expected warnings to conftest.py?

It's similar, but not the same message. I think keeping where it is now would be best. However, this brings a larger discussion of it we should review and generalize the error messages. If we think that should be the case let's open a fresh issue for that.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with opening a fresh issue. I think we can keep these warnings without generalizing them first. To avoid over-engineering, we can do this transition step-by-step. Firstly, move the warnings in conftest. Then, if we see conftest has a lot of repeated code, we should generalize it.

@jGaboardi
Copy link
Member Author

I noted some warnings were not moved to conftest.py. Is there a specific reason for deciding which test should use the conftest.py warning?

  • Yeah, my general rule was that if identical warns/raises happen across testing file then put it in conftest.py. There is one raises that happens twice within test_knearest so I just kept that one in the file itself.
  • After looking through again, I have missed a few cases. Good catch on that! I'll push up some changes.

@jGaboardi
Copy link
Member Author

About the numpy deprecationWarning, I think we should filter it, as it seems, it will disappear in future releases of pyproj.

OK, I will see about adding that to this PR.

@jGaboardi
Copy link
Member Author

@gegen07 Seems I missed quite few 😅.

Also, I am having trouble pinning down where the DeprecationWarning is being thrown. It happens when I run the tests through pytest like usual, but then nothing is thrown after I broke out the code line by line 🤔

@jGaboardi
Copy link
Member Author

OK! I think I've found to offending line. Will push up the fix soon.

@jGaboardi
Copy link
Member Author

@gegen07 OK. I think this is ready for a proper review.

@gegen07 gegen07 merged commit de68d07 into pysal:main Apr 30, 2024
11 checks passed
@jGaboardi jGaboardi deleted the geometry_crs_warning branch April 30, 2024 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants