openclimatefix / Satip

Satip contains the code necessary for retrieving, transforming and storing EUMETSAT data
https://satip.readthedocs.io/
MIT License
41 stars 29 forks source link

Split unit tests and integration tests, and update workflows #264

Closed Jacqueline-J closed 1 month ago

Jacqueline-J commented 1 month ago

This pull request addresses issue #263 "Split integration tests and unit tests"

peterdudfield commented 1 month ago

Thanks so much for doing this, I think its just one small change, to change the files names to have test at the start.

Ill have a think how we can try to test this all works before merging.

Oh and then's one small fix in the pre-commit.cr error

Jacqueline-J commented 1 month ago

Hi Peter,

Initially, I named the files using prefixes like "integration_test" or "unit_test." However, upon running checks, I noticed that no tests were being detected or executed.

After reviewing the “Conventions for Python test discovery”, I found that file names should either start with test_.py or end with _test.py. I changed the name of all the test files and the code passed the checks successfully. But, I understand the importance of adhering to established naming conventions and best practices, so I'm more than willing to make further adjustments by changing the names to start with "test".

Jacqueline-J commented 1 month ago

Thanks so much for doing this, I think its just one small change, to change the files names to have test at the start. Ill have a think how we can try to test this all works before merging.

Oh and then's one small fix in the pre-commit.cr error

Took awhile but I found the issue, it should be fine now :)

peterdudfield commented 1 month ago

Thanks @Jacqueline-J , I'm gona merge this to a branch internal first. Just so we can make sure the integration tests work.

One small things I do that, becasue the coverage will only look at integration tests, I wonder could

  1. Try to merge coverage reports, using something like this - https://pytest-cov.readthedocs.io/en/latest/readme.html#coverage-data-file
  2. run both unit and integration tests second, then the coverage would have all the tests in it
Jacqueline-J commented 1 month ago

@peterdudfield sorry for the oversight! I've appended the code to combine the coverage reports

peterdudfield commented 1 month ago

@peterdudfield sorry for the oversight! I've appended the code to combine the coverage reports

No problem, thanks for doing that. Let me know when your happy and i'll merge (perhaps into a different branch) to check it works all ok

peterdudfield commented 1 month ago

Looks ready to go, shall I merge it into development and then we can see the unit and integration tests work?

Thanks again for all this work

Jacqueline-J commented 1 month ago

Sorry for the slight delay! Everything should be good now. Please go ahead and merge it.