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

Update test_notebook.ipynb to include tests #4

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

jsignell
Copy link
Collaborator

@jsignell jsignell commented Jun 3, 2024

Does some mild testing of:

  • xarray
  • geopandas
  • fsspec
  • pystac_client
  • hvplot

closes NASA-IMPACT/veda-jupyterhub#27

* xarray
* geopandas
* fsspec
* pystac_client
* hvplot
@batpad
Copy link
Collaborator

batpad commented Jun 5, 2024

@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 catalog when opening with STAC. So these lines:

STAC_API_URL = "https://staging-stac.delta-backend.com/"
catalog = Client.open(STAC_API_URL)
catalog

I worry that our tests would break if anything about the catalog at https://staging-stac.delta-backend.com/ changes? Could we maybe only test the output of something more specific / or a query for a time-range that is unlikely to change?

Overall, this looks excellent and just what we needed to get going :-) Thanks again @jsignell !

@sunu
Copy link
Collaborator

sunu commented Jun 5, 2024

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.

@sunu sunu closed this Jun 5, 2024
@sunu sunu reopened this Jun 5, 2024
@jsignell
Copy link
Collaborator Author

jsignell commented Jun 5, 2024

I think the only concern I have is with checking the output of catalog when opening with STAC.

Yes that is a very good point 🙈 maybe I should just do a repr of the item.

@batpad
Copy link
Collaborator

batpad commented Jun 6, 2024

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.

@batpad
Copy link
Collaborator

batpad commented Jun 17, 2024

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 pytest-notebook, or write specific pytest tests to test specific things.

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 pytest-notebook, but this is all very useful learning and should give us things to look into improving going forward.

@jsignell
Copy link
Collaborator Author

No worries! I can pare down the notebook later this week.

@jsignell
Copy link
Collaborator Author

Ok this version passes for me locally. I added some metadata to skip some of the output on import hvplot.xarray but we might want to skip more of the output from that one.

@batpad
Copy link
Collaborator

batpad commented Jun 20, 2024

@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!

@batpad
Copy link
Collaborator

batpad commented Jun 21, 2024

Am going ahead and merging this -

I imagine:

  • As we perform upgrades to the base image, etc. we might find tests that are overly brittle and that we might want to modify
  • We probably want to continue improving the test coverage, and figuring out being able to test for more complex visualizations, etc.

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!

@batpad batpad merged commit d510695 into NASA-IMPACT:main Jun 21, 2024
1 check passed
@jsignell jsignell deleted the js/test-notebook branch June 21, 2024 15:59
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.

Create test notebooks to test expected functionality after image updates
3 participants