nteract / bookstore

📚 Notebook storage and publishing workflows for the masses
https://bookstore.readthedocs.io
BSD 3-Clause "New" or "Revised" License
202 stars 23 forks source link

Add test VersionHandler; Update Version Handler's API #130

Closed mpacer closed 5 years ago

mpacer commented 5 years ago

This refactors a bit of the data manipulation code in the handlers into individually testable utils.

This adds tests for the BookstoreVersionHandler.

There are a lot of issues that became obvious as I wrote these tests & helper utils. I will create separate issues for them, but I wanted to list them here.

  1. We should move BookstoreVersionHandler outside of the handlers.py module, into it's own module.
  2. We could simplify the code without much work. But, the response is being used for validation by front-ends (e.g., nteract_on_jupyter), and so we will need to make sure that this works for them as well. Specifically, this would involve removing the "bookstore": True bit in the response from the BookstoreVersionHandler
  3. Our tests are not all correctly awaiting the asynchronous handler methods, this means our synchronous code in those methods should be tested appropriately, but we'll need to add some more stuff to the other tests to get that working.
todo[bot] commented 5 years ago

Shuld we remove this; isn't it equivalent to the endpoint responding?

https://github.com/nteract/bookstore/blob/aa4e1e3660d268a931e1dab3bd2d760c3c246447/bookstore/handlers.py#L43-L48


This comment was generated by todo based on a TODO comment in aa4e1e3660d268a931e1dab3bd2d760c3c246447 in #130. cc @mpacer.
todo[bot] commented 5 years ago

Should we remove this; isn't it equivalent to the endpoint responding?

https://github.com/nteract/bookstore/blob/e1d393bc332208a7706581bc9069179e9aa9edbc/bookstore/handlers.py#L43-L48


This comment was generated by todo based on a TODO comment in e1d393bc332208a7706581bc9069179e9aa9edbc in #130. cc @mpacer.
codecov[bot] commented 5 years ago

Codecov Report

Merging #130 into master will increase coverage by 0.82%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   47.32%   48.14%   +0.82%     
==========================================
  Files          11       11              
  Lines         374      378       +4     
==========================================
+ Hits          177      182       +5     
+ Misses        197      196       -1
codecov[bot] commented 5 years ago

Codecov Report

Merging #130 into master will increase coverage by 0.43%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   79.14%   79.57%   +0.43%     
==========================================
  Files          10       10              
  Lines         422      426       +4     
==========================================
+ Hits          334      339       +5     
+ Misses         88       87       -1
todo[bot] commented 5 years ago

Should we remove this; isn't it equivalent to the endpoint responding?

https://github.com/nteract/bookstore/blob/1489aecd511e4bf7b415f9f70a9b46747d9c68bd/bookstore/handlers.py#L43-L48


This comment was generated by todo based on a TODO comment in 1489aecd511e4bf7b415f9f70a9b46747d9c68bd in #130. cc @mpacer.
mpacer commented 5 years ago

Hi @MSeal @willingc I made the changes to the API that you suggested as part of this PR now.

this has been rebased on master & is ready for the next round of reviews.

mpacer commented 5 years ago

@willingc I'm sorry I missed your comment about changing the name of the top level property of the API response from "validation" to "features" — I just caught it and made the change

willingc commented 5 years ago

I don't see any outstanding issues in this PR so I am going to merge. Thanks @mpacer 🌻

mpacer commented 5 years ago

Thanks!