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

mpacer · 2019-06-11T03:54:52Z

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.

We should move BookstoreVersionHandler outside of the handlers.py module, into it's own module.
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
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 · 2019-06-11T03:54:54Z

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

bookstore/bookstore/handlers.py

Lines 43 to 48 in aa4e1e3

    
               "bookstore": True, # TODO: Shuld we remove this; isn't it equivalent to the endpoint responding? 
        
               "version": self.settings['bookstore']["version"], 
        
               "validation": self.settings['bookstore']["validation"], 
        
           }

This comment was generated by todo based on a `TODO` comment in `aa4e1e3` in #130. cc @mpacer.

todo · 2019-06-11T16:44:31Z

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

bookstore/bookstore/handlers.py

Lines 43 to 48 in e1d393b

    
               "bookstore": True,  # TODO: Should we remove this; isn't it equivalent to the endpoint responding? 
        
               "version": self.settings['bookstore']["version"], 
        
               "validation": self.settings['bookstore']["validation"], 
        
           }

This comment was generated by todo based on a `TODO` comment in `e1d393b` in #130. cc @mpacer.

codecov · 2019-06-11T16:51:43Z

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 · 2019-06-11T16:51:43Z

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

bookstore/handlers.py

bookstore/tests/test_handlers.py

MSeal · 2019-06-12T03:02:02Z

bookstore/tests/test_handlers.py

+        """
+        get_handler = self.get_handler('/api/bookstore/')
+        setattr(get_handler, '_transforms', [])
+        get_handler.get()


We don't assert anything. There's a lot of code issues that would appear to pass this test when they are in fact not doing what's intended

We're not doing this with all of the other handler tests. I agree it would be better, but I am not aiming at ideal right now.

What are those code issues?

What if the get returns None? Or get_handler returns the post handler? This test as is doesn't confirm much.

I think the get should return None given the way that tornado handlers are defined (it treats the finish as a side-effect that I'm not sure how to hook into).

What it confirms is that the code runs without raising an error, meaning we've met the API for the get method, and that such a handler can be created without issue.

The problem is there is no way that I can seem to get access to whatever is sent over the wire based on get.

bookstore/tests/test_handlers.py

willingc

A couple of API design comments. It feels a bit squishy to have release version and validation together. Would it be preferable to have releases, features (validation seems more like something that has been done. From an api user standpoint, would it be more beneficial for me to know which features are possible, available, valid, and their settings?).

bookstore/handlers.py

todo · 2019-06-27T18:52:07Z

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

bookstore/bookstore/handlers.py

Lines 43 to 48 in 1489aec

    
               "bookstore": True,  # TODO: Should we remove this; isn't it equivalent to the endpoint responding? 
        
               "version": self.settings['bookstore']["version"], 
        
               "validation": self.settings['bookstore']["validation"], 
        
           }

This comment was generated by todo based on a `TODO` comment in `1489aec` in #130. cc @mpacer.

mpacer · 2019-06-27T19:23:18Z

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.

This is used inside the load_server_extension function to populate the settings dictionary for access in the handler itself.

This requires patching the handler's _transforms method to avoid raising an error when we run self.finish() on the handler code side. This is likely an issue that we needed to deal with for other handlers but have not appropriately done. This is something we should revisit.

- suggestion originates from code review

mpacer · 2019-06-29T02:27:45Z

@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 · 2019-07-02T14:31:21Z

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

mpacer · 2019-07-02T17:04:37Z

Thanks!

This was referenced Jun 11, 2019

Move BookstoreVersionHandler to separate module from handlers #131

Open

Alter BookstoreVersionHandler response #132

Open

Ensure that finish is called in async RequestHandler tests #133

Open

mpacer force-pushed the testing branch from aa4e1e3 to e1d393b Compare June 11, 2019 16:44

mpacer requested review from willingc and MSeal June 11, 2019 16:45

MSeal reviewed Jun 12, 2019

View reviewed changes

bookstore/handlers.py Outdated Show resolved Hide resolved

MSeal reviewed Jun 12, 2019

View reviewed changes

bookstore/tests/test_handlers.py Show resolved Hide resolved

MSeal reviewed Jun 12, 2019

View reviewed changes

bookstore/tests/test_handlers.py Outdated Show resolved Hide resolved

willingc reviewed Jun 12, 2019

View reviewed changes

bookstore/handlers.py Outdated Show resolved Hide resolved

mpacer mentioned this pull request Jun 15, 2019

Refactor & Test Validation and Handler Adding #134

Merged

mpacer force-pushed the testing branch from e1d393b to 1489aec Compare June 27, 2019 18:52

mpacer force-pushed the testing branch 2 times, most recently from 84a6a23 to bfeecf1 Compare June 27, 2019 19:22

mpacer changed the title ~~Add tests for handlers; specifically VersionHandler~~ Add test VersionHandler; Update Version Handler's API Jun 28, 2019

mpacer requested a review from rgbkrk June 28, 2019 21:48

mpacer added this to the 2.3 milestone Jun 28, 2019

mpacer added 6 commits June 28, 2019 18:58

Isolate building the response so it can be independently tested

0e551de

Refactor building_settings_dict for independent testing

dfdc215

This is used inside the load_server_extension function to populate the settings dictionary for access in the handler itself.

Create fixture for bookstore settings

e415f55

Create test for build_settings_dict using bookstore_settings fixture

927bb85

Update fixture to also be accessible via optional class access

8a3ea41

Set up initial handler testing framework; follow test_clone pattern

9907f29

mpacer added 6 commits June 28, 2019 18:58

Add test for independent build_response code.

a66e8f0

Improve documentation for bookstore's BookstoreVersionHandler methods.

91192f1

Update API docs to match validation info

c474a6e

Update test structure & expected values and remove empty test

8dbf4e1

Switch to dynamically pulling the version rather than hard coding it

7f98456

- suggestion originates from code review

mpacer force-pushed the testing branch from cf2e8fb to 41115d1 Compare June 29, 2019 01:59

mpacer added 2 commits June 28, 2019 19:02

Update API response based on requested changes in code reviews

8d48e55

Add expected None to handler's get test

c9fde2d

mpacer force-pushed the testing branch from 41115d1 to c9fde2d Compare June 29, 2019 02:02

Change API from "validation" to "features"

f71b812

willingc merged commit 7d9a8a5 into nteract:master Jul 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mpacer commented Jun 11, 2019

todo bot commented Jun 11, 2019

todo bot commented Jun 11, 2019

codecov bot commented Jun 11, 2019

codecov bot commented Jun 11, 2019 •

edited

Loading

MSeal Jun 12, 2019 •

edited

Loading

mpacer Jun 12, 2019

mpacer Jun 27, 2019

MSeal Jun 28, 2019

mpacer Jun 29, 2019

willingc left a comment

todo bot commented Jun 27, 2019

mpacer commented Jun 27, 2019

mpacer commented Jun 29, 2019

willingc commented Jul 2, 2019

mpacer commented Jul 2, 2019

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

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

Conversation

mpacer commented Jun 11, 2019

todo bot commented Jun 11, 2019

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

This comment was generated by todo based on a TODO comment in aa4e1e3 in #130. cc @mpacer.

todo bot commented Jun 11, 2019

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

This comment was generated by todo based on a TODO comment in e1d393b in #130. cc @mpacer.

codecov bot commented Jun 11, 2019

Codecov Report

codecov bot commented Jun 11, 2019 • edited Loading

Codecov Report

MSeal Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

mpacer Jun 12, 2019

Choose a reason for hiding this comment

mpacer Jun 27, 2019

Choose a reason for hiding this comment

MSeal Jun 28, 2019

Choose a reason for hiding this comment

mpacer Jun 29, 2019

Choose a reason for hiding this comment

willingc left a comment

Choose a reason for hiding this comment

todo bot commented Jun 27, 2019

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

This comment was generated by todo based on a TODO comment in 1489aec in #130. cc @mpacer.

mpacer commented Jun 27, 2019

mpacer commented Jun 29, 2019

willingc commented Jul 2, 2019

mpacer commented Jul 2, 2019

This comment was generated by todo based on a `TODO` comment in `aa4e1e3` in #130. cc @mpacer.

This comment was generated by todo based on a `TODO` comment in `e1d393b` in #130. cc @mpacer.

codecov bot commented Jun 11, 2019 •

edited

Loading

MSeal Jun 12, 2019 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `1489aec` in #130. cc @mpacer.