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

Fix test failures in test_granules.py #61

Merged
merged 5 commits into from
Jun 26, 2020
Merged

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented Jun 4, 2020

Pytest automatically tests any files starting with the name test_, in folders under tests, see https://docs.pytest.org/en/latest/example/pythoncollection.html. This means we don't need to explicitly add new tests to the .travis.yml file everytime.

TODO:

  • Let travis test everything
  • Fix failing tests in test_granule.py

@weiji14
Copy link
Member Author

weiji14 commented Jun 4, 2020

This is the remaining test failure:

=================================== FAILURES ===================================
______________________ test_correct_granule_list_returned ______________________

    def test_correct_granule_list_returned():
        reg_a = ipd.Icesat2Data('ATL06',[-55, 68, -48, 71],['2019-02-20','2019-02-28'], version='2')
        reg_a.avail_granules()
>       obs_grans = [gran['producer_granule_id'] for gran in reg_a.granules]
E       TypeError: 'Granules' object is not iterable

icepyx/tests/test_granules.py:407: TypeError

Not sure if it's meant to test reg_a.avail_granules(ids=True) or something else?

@weiji14
Copy link
Member Author

weiji14 commented Jun 8, 2020

Ok, found out that the test_correct_granule_list_returned should be using reg_a.granules.avail instead of reg_a.granules. Should be all good now!

Copy link
Member

@JessicaS11 JessicaS11 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing those granules tests! icepyx underwent a large scale refactor about a month ago, so I haven't finished porting all of the tests over to the new structure (which is why not all of them are run yet).

I am aware that pytest will run all files that start with test. However, the CI is specifically set up in stages because different sets of tests are run under different circumstances. At the moment the second stage is disabled (see previous statement about the major refactor), but it will eventually be turned on again. Unfortunately, I had to merge the changes in this branch (which included a few bug fixes) before I got to where I wanted to be with the test updating because I needed them to be in master for the ongoing Hackweek and didn't have time to figure out which commits they were in. Maybe not the best practice, but I'm still learning and refining my own system as I go! If you could revert the changes to .travis.yml to keep the staged testing, then I will go ahead and merge this PR!

@weiji14 weiji14 changed the title Let travis test everything in icepyx Fix test failures in test_granules.py Jun 9, 2020
@weiji14
Copy link
Member Author

weiji14 commented Jun 9, 2020

Ok, I've reverted the change (and changed the title too). But before you merge this, what I can do is to set travis up so that the first stage tests everything except tests/behind_NSIDC_API_login.py using the --ignore option in pytest as documented at https://docs.pytest.org/en/latest/example/pythoncollection.html#ignore-paths-during-test-collection. If that's what all the second stage is about (testing functions that require a login)?

Alternatively, we could put all the 'second stage' tests in a folder like tests/login_tests, and have pytest ignore those ones in the first stage, and test those ones in the second stage. Or you could set up a test matrix so that both of the stages are run concurrently (which might help with #38).

As an aside, I found https://automationpanda.com/2017/03/06/python-testing-101-introduction/ to be a good resource for learning about testing when I first started (maybe check it out after the Hackweek), the pytest specific one at https://automationpanda.com/2017/03/14/python-testing-101-pytest/ might be a good one to focus on 😄

@JessicaS11
Copy link
Member

@weiji14 I am fine with using the pytest --ignore option for the first suite of tests (basic tests stage). One of the issues I think we will keep running into in the short term is that the test suite is not yet completed (and has only been partially brought over from a previous version of the code, which is why not all of the test files start with "test" - that's the indicator that they still need updating!). My thought was that once we have a more complete set of tests it will be easier to determine which of them should be put into test matrices (as per #38 --> I intended for this to apply to the basic tests stage, which is currently what takes the bulk of the test time).

Thanks for those resources - I look forward to checking them out when this crazy week is over!

@weiji14
Copy link
Member Author

weiji14 commented Jun 16, 2020

Cool, I've used --ignore to skip the behind_NSIDC_API_login.py. I've only just noticed it didn't start with test_, which means they wouldn't actually be tested 😲! It should be fine with the short term (not testing everything), but once we get into refactoring code, then it would be best to have a proper test suite.

Great job on the Hackweek by the way, I've just been going over a few of the youtube videos and I can only imagine all the work going behind them to make everything go so smoothly! Definitely get some rest after, it's well deserved 😁

@JessicaS11
Copy link
Member

Yup - taking "test" out of the filename was very intentional until I got to updating it! Thanks for these changes and the kudos. The Hackweek went quite well, but I'm still catching up on everything that happened during it (or I missed because of it). : )

@JessicaS11 JessicaS11 changed the base branch from master to development June 26, 2020 18:50
@JessicaS11 JessicaS11 merged commit 82f4aec into development Jun 26, 2020
@JessicaS11 JessicaS11 deleted the travis_test_all branch June 26, 2020 18:52
@JessicaS11
Copy link
Member

@weiji14 Don't forget to add yourself as a contributor on your next PR!

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.

2 participants