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

Refactor test_area and move boundary related tests to test_area_boundary #564

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

ghiggi
Copy link
Contributor

@ghiggi ghiggi commented Dec 8, 2023

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Passes git diff origin/main **/*py | flake8 --diff
  • Fully documented

This PR

  • extract the areas defined in the test units as fixtures into areas.py
  • move the TestBoundary and TestGeostationaryTools classes from test_area.py into a new test_area_boundary.py file.
  • move the TestSwathBoundary and TestSwathBboxLonLats classes from test_swath.py into a new test_swath_boundary.py file.

This refactor reduce the code length of such test units and facilitate the addition of new test units in the upcoming PRs.

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (c240c04) 94.09% compared to head (8d19a4e) 93.88%.
Report is 10 commits behind head on main.

Files Patch % Lines
pyresample/test/test_geometry/conftest.py 74.54% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   94.09%   93.88%   -0.21%     
==========================================
  Files          85       88       +3     
  Lines       13235    13342     +107     
==========================================
+ Hits        12453    12526      +73     
- Misses        782      816      +34     
Flag Coverage Δ
unittests 93.88% <91.78%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiggi
Copy link
Contributor Author

ghiggi commented Dec 8, 2023

@mraspaud @djhoese ready for review ;)

@coveralls
Copy link

coveralls commented Dec 8, 2023

Coverage Status

coverage: 93.473% (-0.2%) from 93.675%
when pulling 8d19a4e on ghiggi:refactor-test-area
into c240c04 on pytroll:main.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mraspaud
Copy link
Member

mraspaud commented Dec 8, 2023

Regarding coverage of the fixtures, are we sure they are used?

@ghiggi
Copy link
Contributor Author

ghiggi commented Dec 8, 2023

@mraspaud a couple of these fixtures are not yet used but will be in one of the next PRs.
Please let me pass this ... I am already trying to keep track of hundreds of stuffs :)
Can I move the TestBoundary and TestGeostationaryTools classes into a new test_area_boundary.py file?

@mraspaud
Copy link
Member

mraspaud commented Dec 8, 2023

@mraspaud a couple of these fixtures are not yet used but will be in one of the next PRs. Please let me pass this ... I am already trying to keep track of hundreds of stuffs :)

I was suspecting as much. Fine by me.

Can I move the TestBoundary and TestGeostationaryTools classes into a new test_area_boundary.py file?

Yes. Sorry I thought this PR was ready. If not, make sure to check the Draft option. I almost merged this earlier.

@ghiggi
Copy link
Contributor Author

ghiggi commented Dec 8, 2023

@djhoese also this one I guess is now ready to be merged ;) Have a nice week-end

@djhoese
Copy link
Member

djhoese commented Dec 8, 2023

Lots of pre-commit failures

@ghiggi
Copy link
Contributor Author

ghiggi commented Dec 10, 2023

@djhoese It seems that flake8 does not recognize the area fixtures imported and raises F811 errors

@djhoese
Copy link
Member

djhoese commented Dec 10, 2023

I will look later.

Pytest fixtures can't be imported. They must be local (in the module) or in conftest.py somewhere
@djhoese
Copy link
Member

djhoese commented Dec 11, 2023

Ok. Pre-commit is happy, but we'll see if all the tests made it through my reworking. Bottom line is that pytest fixtures are magic. You can't import them. They either have to be defined in the current scope (module, class, etc) or in a special conftest.py module. The conftest.py module can be in the primary pyresample/test/ directory, but also additionally in each sub-directory of the pyresample/test/ module (ex. test_geometry). This module is automatically run/included when running tests under this particular directory and both the root test/conftest.py and the subdir test/test_geometry/conftest.py are included/loaded.

One thing to consider would be modifying the scope of these fixtures by doing pytest.fixture(scope="session") if we consider them read-only/immutable so that they aren't wastefully recreated every test. It could also be package scope if package means "sub-package", but I'm not sure on that.

https://docs.pytest.org/en/6.2.x/fixture.html#fixture-scopes

@ghiggi
Copy link
Contributor Author

ghiggi commented Dec 12, 2023

Oh I didn't know that fixtures couldn't be imported. Good to know ... learning new stuffs everyday 😄

I try to use pytest.fixture(scope="session") but I got:
ScopeMismatch: You tried to access the function scoped fixture create_test_area with a session scoped request object, involved factories

Any idea on what is going on?
If we use pytest.fixture(scope="session") we also implicitly test that modification in place does not occur right? We don't risk to have problems when we test i.e. caching (i.e on class attributes?)

@djhoese
Copy link
Member

djhoese commented Dec 12, 2023

Ah, forget the scoping then. Although...I suppose create_test_area could also be scoped to session... 🤷‍♂️ I'm not sure it is worth the effort. Especially in this PR. And yes, we assume that the areas aren't being modified in place (which is generally prohibited by checks in the classes properties).

For the error you were getting: it is saying that create_test_area which is rerun/recreated for every test function is being used by the area fixtures you're making which are session scoped and created once. That logically doesn't make sense: you can't have something that is created once use something that should update every function.

@djhoese djhoese merged commit cdb8fb4 into pytroll:main Dec 14, 2023
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants