Closed lahdjirayhan closed 2 years ago
Great PR! Gonna be really awesome to have a testing suite, and this one looks really comprehensive :)
Should we use tox for doing the tests? Especially in light of error discovered in https://github.com/Milind220/Ozone/issues/119
I'm going to read up on tox, seeing as I don't currently know what it is. I'm assuming it helps catch errors that may crop up in the process of packaging and importing?
Some caveats about the tests and cassettes:
"-"
a string, with just a hyphen in it. I get that it's supposed to mean NA, maybe, but it throws a ValueError: could not convert string to float: '-'
that I've never seen before.get_range_coordinates_air
, and you'll see (I compare it with v1.5.0
tag, there was no error there). This error is likely missed when refactoring the code.My suggestion to go about this:
@Milind220 Given we're both new to tox, I think we should focus on the test suite as it is and think about it in another time. Sorry for raising it earlier, it felt like I've bothered you and overcomplicated the PR unnecessarily. 🙏🏼
- There is an uncaught bug in current Ozone! Try using
get_range_coordinates_air
, and you'll see (I compare it withv1.5.0
tag, there was no error there). This error is likely missed when refactoring the code.
Oh yikes! Great that we have tests now though :)
My suggestion to go about this:
- I recommend fixing Ozone to pass the tests than the other way around. They're mostly minor aspects, but please do check the failing tests anyway.
I absolutely agree, tests will ensure that Ozone's code is up to a decent standard.
- I recommend fixing Ozone to handle such edge cases, and leave the cassettes as it is (not re-record). Let's just say we're lucky to have an edge case in our cassette. But re-recording them would be fine anyway.
- This has to be fixed asap.
Let's leave the cassettes as they are, and fix the bugs asap for sure.
@Milind220 Given we're both new to tox, I think we should focus on the test suite as it is and think about it in another time. Sorry for raising it earlier, it felt like I've bothered you and overcomplicated the PR unnecessarily. 🙏🏼
No worries, we can always add it in another time.
Awesome work! I went through the tests and they look really solid and well formulated. Can't wait to see Ozone'd code quality improve with the tests checking :)
@lahdjirayhan Just to confirm, this PR is complete right? I do have a couple questions.
Just to confirm, this PR is complete right?
Yes. Feel free to merge if you think it's enough.
What is the current mechanism to run these tests? Do we run them on locally on our own computers?
Yes. Each developer can verify and test their work locally and automatically (not manually test each cases one by one).
We still have to add continuous testing integration right?
Yep. It's not included in this PR.
This PR is currently working in progress, to fix #8.
List of public methods to write tests of:
get_city_air
get_city_forecast
get_city_station_options
get_coordinate_air
get_historical_data
get_multiple_city_air
get_multiple_coordinate_air
get_range_coordinates_air
get_specific_parameter
reset_token
?In summary:
pytest
as the test frameworkpytest-recording
plugin to utilizevcr.py
for mocking API response dataThis PR should also:
Potential discussion points:
tox
for doing the tests? Especially in light of error discovered in #119