Skip to content

Conversation

@MridulS
Copy link
Contributor

@MridulS MridulS commented Feb 11, 2021

A regression was introduced in #993 which broke the Dataverse build process.

This should fix jupyterhub/binderhub#1263 (the repo builds locally now).

also should there be a test for this?

@welcome
Copy link

welcome bot commented Feb 11, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@minrk
Copy link
Member

minrk commented Feb 11, 2021

I believe this is tested, but the response is mocked and the same mistake was introduced to the test as well, so the code matches the mock, but not the real thing. The 'data' that was here is missing here. I think if you update the mocks with the same patch, this should be passing agiain.

This is the trouble with mocks in tests! They only test that your code works with your mocks, not whether your mocks are accurate :/

@sgibson91
Copy link
Member

This is the trouble with mocks in tests! They only test that your code works with your mocks, not whether your mocks are accurate :/

I literally suffered a whole new wave of imposter syndrome when I first started learning about mocking!

We have CI repos on GitHub and GitLab, could we have one on Dataverse too or is it a "host your own server" thing?

@MridulS
Copy link
Contributor Author

MridulS commented Feb 11, 2021

Ah yeah, thanks for the catch!

The test_env seems like an unrelated test fail.

@pdurbin
Copy link
Contributor

pdurbin commented Feb 11, 2021

First of all, thanks for fixing this!! 🎉

Does repo2docker need real DOIs? If not, you're welcome to use https://demo.dataverse.org for testing. 😄

@minrk minrk merged commit 8582d44 into jupyterhub:master Feb 12, 2021
@welcome
Copy link

welcome bot commented Feb 12, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@minrk
Copy link
Member

minrk commented Feb 12, 2021

Merging since this fixes the issue. The env test is unrelated, we'll need to figure out if it was just a flaky run or will keep failing.

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.

Launch binder doesn't work for Dataverse DOI

4 participants