-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update test_notebook.ipynb to include tests #4
Conversation
* xarray * geopandas * fsspec * pystac_client * hvplot
@jsignell thank you so much!! 🚀 🚀 @sunu - would you be able to do a review here, and test that things look good and merge? From a glance through the notebook, I think the only concern I have is with checking the output of
I worry that our tests would break if anything about the catalog at Overall, this looks excellent and just what we needed to get going :-) Thanks again @jsignell ! |
I configured the GitHub Actions workflow to run builds and tests automatically upon pull request creation in #5. Let me close and reopen this PR to trigger the build. |
Yes that is a very good point 🙈 maybe I should just do a repr of the item. |
CI is behaving a bit weird, filed #6 - things work fine for me locally so not sure what's going on. We will debug that. |
Hm sorry, as it stands, it seems like #6 is likely a blocker for doing more complex visualization tests. I think this would be good to figure out in the future, but I think for now we should whittle these tests down a bit and even if the code is complex, restrict the Outputs being tested to string representations. For other things, we either would need to see if there's a way to get this working in I definitely think we need to continue working on improving the tests, including being able to run tests in cluster with s3 bucket permissions etc. For now, @jsignell, do you know if you might have time to reduce the scope of the test notebook a bit to stick to string outputs for now? No worries if not, then will just make sure we prioritize this this week between me and @sunu - thanks again for your work here and really sorry we did not realize this limitation with |
No worries! I can pare down the notebook later this week. |
Ok this version passes for me locally. I added some metadata to skip some of the output on |
@jsignell you're the best! 🎉 This looks like it's passing CI as well! I'll just give things a quick look tomorrow and merge. And then we can test upgrading to the latest pangeo base image and see how that goes. Thanks again! |
Am going ahead and merging this - I imagine:
I think this is a great start and a significant improvement from what we had earlier (no tests). @jsignell as we frame PI objectives for the next quarter, it would be great to discuss what we want to do here, and it would be amazing if we were able to continue getting some of your time to think about and improve tests here. Ofc, feel free to continue making any changes / improvements here as you see fit, but I definitely think this is good to merge. Thanks @jsignell and @sunu for your work here! |
Does some mild testing of:
closes NASA-IMPACT/veda-jupyterhub#27