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

Add tests suite #120

Merged
merged 18 commits into from
Apr 22, 2022
Merged

Add tests suite #120

merged 18 commits into from
Apr 22, 2022

Conversation

lahdjirayhan
Copy link
Contributor

@lahdjirayhan lahdjirayhan commented Apr 21, 2022

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:

  • use pytest as the test framework
  • use pytest-recording plugin to utilize vcr.py for mocking API response data
  • one test file per public method

This PR should also:

  • add instructions to run tests (preferably in CONTRIBUTING)
  • add guidelines to add or modify tests, esp. about when will it be necessary to do so
  • add guidelines about VCR.py cassettes

Potential discussion points:

@Milind220
Copy link
Collaborator

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 #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?

@Milind220 Milind220 linked an issue Apr 21, 2022 that may be closed by this pull request
@lahdjirayhan lahdjirayhan marked this pull request as ready for review April 22, 2022 03:21
@lahdjirayhan
Copy link
Contributor Author

Some caveats about the tests and cassettes:

  1. The tests are not written in a way that current Ozone will just pass. I've used my best judgments when exploring/writing the tests. It feels different when writing tests vs when writing code.
  2. Some cassettes are (unluckily? luckily?) getting some quirky edge cases that throws error. I've never encountered this behavior in particular: Paris' AQI is given out as "-" 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.
  3. There is an uncaught bug in current Ozone! Try using 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:

  1. 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.
  2. 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.
  3. This has to be fixed asap.

@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. 🙏🏼

@Milind220
Copy link
Collaborator

  1. There is an uncaught bug in current Ozone! Try using 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.

Oh yikes! Great that we have tests now though :)

My suggestion to go about this:

  1. 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.

  1. 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.
  2. 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 :)

Copy link
Collaborator

@Milind220 Milind220 left a comment

Choose a reason for hiding this comment

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

Tests look really great - I think we'll find a lot more wrong with Ozone - which is a good thing!

@pytest.mark.vcr
def test_bad_coordinates():
with pytest.raises(Exception):
api.get_range_coordinates_air(lower_bound=("lol", "bruh"), upper_bound=(0, 0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

hahahaha 'lol' and 'bruh'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Short enough
  • Not interpretable as a valid coordinate value

So ... hehe.

@Milind220
Copy link
Collaborator

@lahdjirayhan Just to confirm, this PR is complete right? I do have a couple questions.

  • What is the current mechanism to run these tests? Do we run them on locally on our own computers?
  • We still have to add continuous testing integration right?

@lahdjirayhan
Copy link
Contributor Author

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.

@Milind220 Milind220 merged commit 3ec164d into Ozon3Org:dev Apr 22, 2022
@lahdjirayhan lahdjirayhan deleted the add-tests branch April 22, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addition of tests to check build status
2 participants