pytroll / pyresample

Geospatial image resampling in Python
http://pyresample.readthedocs.org
GNU Lesser General Public License v3.0
343 stars 95 forks source link

Refactor ``test_area`` and move boundary related tests to ``test_area_boundary`` #564

Closed ghiggi closed 7 months ago

ghiggi commented 7 months ago

This PR

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

codecov[bot] commented 7 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/pytroll/pyresample/pull/564/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/pytroll/pyresample/pull/564/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `93.88% <91.78%> (-0.21%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ghiggi commented 7 months ago

@mraspaud @djhoese ready for review ;)

coveralls commented 7 months ago

Coverage Status

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

mraspaud commented 7 months ago

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

ghiggi commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

djhoese commented 7 months ago

Lots of pre-commit failures

ghiggi commented 7 months ago

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

djhoese commented 7 months ago

I will look later.

djhoese commented 7 months ago

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 commented 7 months ago

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

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 commented 7 months ago

Ah, forget the scoping then. Although...I suppose create_test_area could also be scoped to session... :man_shrugging: 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.