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

Cleanup tests #85

Merged
merged 7 commits into from
Jan 11, 2023
Merged

Cleanup tests #85

merged 7 commits into from
Jan 11, 2023

Conversation

mraspaud
Copy link
Member

As a continuation of #84 , some more grunt work to refactor and cleanup the tests.

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

@mraspaud mraspaud requested a review from djhoese January 10, 2023 12:56
@mraspaud mraspaud self-assigned this Jan 10, 2023
@mraspaud
Copy link
Member Author

Got the file under 2000 lines, 🎉

@coveralls
Copy link

coveralls commented Jan 10, 2023

Coverage Status

Coverage: 94.477% (-0.6%) from 95.051% when pulling 7effb22 on mraspaud:refactor-pycoast-tests into 8589e6d on pytroll:main.

@mraspaud
Copy link
Member Author

@djhoese I can't seem to figure out how to fix the sphinx error. I added the pytest-lazy-fixture to the docs, but the build doesn't seem to catch it. Could it be that the build image is being cached?

@djhoese
Copy link
Member

djhoese commented Jan 10, 2023

Check how I do it in satpy. I think I had to load pytest_lazy_fixture directly and use tha5, not the pytest.plugin version.

@mraspaud
Copy link
Member Author

@djhoese thanks! trying this now.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

If the coverage loss is purely just the changing lines in the test_pycoast.py module then 👌. If it is any part of the contour writer modules then not 👌.

Nice job. Looks much better.

@mraspaud
Copy link
Member Author

I haven't changed anything in the production code, and didn't remove any test functionality, so my guess is that the coverage drop is due to fewer test lines and more production lines. Maybe we should remove the tests from the coverage computation?

@mraspaud mraspaud merged commit a37aeec into pytroll:main Jan 11, 2023
@mraspaud mraspaud deleted the refactor-pycoast-tests branch January 11, 2023 08:50
@djhoese
Copy link
Member

djhoese commented Jan 11, 2023

Maybe we should remove the tests from the coverage computation?

I'm not a fan of this. I feel like it is a quick identifier for when a typo makes a test not run or a utility/helper in a test isn't used anymore.

@djhoese
Copy link
Member

djhoese commented Jan 11, 2023

Coveralls shows that the p_file_path fixture is never used it seems.

@mraspaud
Copy link
Member Author

Coveralls shows that the p_file_path fixture is never used it seems.

Indeed, removing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants