-
Notifications
You must be signed in to change notification settings - Fork 14
Automated testing of notebooks #147
Comments
I like the simplicity of this idea. It will help to evaluate the PRs. Because we already save the However, we should not rely completely on travis-ci green light for merging. Even with the testing scheme in place we should send the nbviewer url with every PR to compare results and check the outputs (specially when updating an existing notebook). PS: For the Here is an example on how to do that: |
I tried to implement something along those lines. (Warning: it is very hackish! We can do better!!) The testing module: And the By implementing this I realized that travis will return several (if not all) failures by hanging up. That is due to the heavy download that happens is almost all the notebooks in Any ideas? Split the tests by mocking the download and use saved files for the second part? |
It looks like Travis times out because it didn't receive output for 10 minutes. So the inundation notebook takes more than 10 minutes to run? It shouldn't. We should be smarter about eliminating models that don't need to be tested. And other notebooks run faster, right? Can set at least get it testing the ones that are fast? |
@rsignell-usgs i think we should remove some of the extreme notebooks from that list as most of them have large data access. |
Yes, or maybe @ocefpaf can figure out how to eliminate some of the models that get accessed but never used (perhaps by being more clever in the script) |
@birdage For now I think we should add all the notebooks to get some idea of which ones will fail. Later on we can start skipping, non-silently though, those notebooks that are hanging-up. @rsignell-usgs The SECOORA inundation notebook minimizes* the download by using the KDTree for searching the data. The download only happens if data are found within the requested distance. There are still some download waste when the data are located over land**. The script does not know if it is land or water until it downloads and checks the data. Maybe I could create a land mask and eliminate all point inside that before hand... Not sure what is the best approach here. Here is the log file with the download times for each model: No data near* takes just a few seconds. land** takes > 1 minute. |
Folks, Is this an enhancement that we can reasonably expect to be implemented within the next 6 weeks or so, or is it a larger effort that must wait for a future project? |
@ocefpaf i think that is a good starting point yes, just wanted to point out that different notebooks may have different response times when it comes to collecting data. |
@jkupiec good question, i think it would be a "nice to have" from an integration stand point, and may help closer to the deadline as we try to get things squared away so we are not scrambling to fix errors from a merge. But on the flip side if we are correctly testing notebooks before merging this wouldnt be as much as an issue. my 2 cents. |
@birdage Can you setup a travis account for |
I just sent a PR (#152) with 3 (actually 2) tests.
I do not know how travis will "react" to this. As soon as someone creates the Travis account we can start playing with this and fix the broken stuff. |
@ocefpaf i spoke to @daf that there isnt really a need to create a travis account, it attaches to the github account , i dont have admin access to the SIT repo so cant flip the switch to turn travis on, but i have it on for my fork of the SIT repo https://travis-ci.org/birdage/system-test |
By "creating the travis account" I meant exactly that: attach the @birdage I did the same for my branch. But if the main |
Travis seems to be doing something, closing this issue. Further issues with Travis can have their own home. |
myself and a few others (@Bobfrat,@daf) have been discussing automated testing of notebooks through travis ci. The testing scope would be pretty limited, i.e just running the notebooks to make sure that no errors are found. This would help when merging, as the merger could see straight away that there might be an issue.
The text was updated successfully, but these errors were encountered: