Test nb_client module #136

mpacer · 2019-06-15T03:01:23Z

This add unit tests for the record types and simple functions in nb_client.

It will either take some clever mocking or proper integration tests to properly handle the rest of the classes in this module.

Going through this made me wonder if we should split this module into an nb_types and and nb_client module… as this seemed to be a fairly clean delineation between the two types of functionality.

todo · 2019-06-15T03:01:26Z

once id is re-added to NotebookSession's attributes readd this

bookstore/bookstore/client/tests/test_nb_client.py

Lines 93 to 98 in 60249b2

    
           # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this 
        
           assert notebook_session.path == session_dict["path"] 
        
           assert notebook_session.name == session_dict["name"] 
        
           assert notebook_session.type == session_dict['type'] 
        
           assert notebook_session.kernel == KernelInfo(**session_dict["kernel"]) 
        
           assert notebook_session.notebook == session_dict["notebook"]

This comment was generated by todo based on a `TODO` comment in `60249b2` in #136. cc @mpacer.

codecov · 2019-06-15T03:31:56Z

Codecov Report

Merging #136 into master will decrease coverage by 1.23%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   47.32%   46.09%   -1.24%     
==========================================
  Files          11       11              
  Lines         374      384      +10     
==========================================
  Hits          177      177              
- Misses        197      207      +10

codecov · 2019-06-15T03:31:56Z

Codecov Report

Merging #136 into master will increase coverage by 18.34%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #136       +/-   ##
==========================================
+ Coverage   50.25%   68.6%   +18.34%     
==========================================
  Files          11      10        -1     
  Lines         386     395        +9     
==========================================
+ Hits          194     271       +77     
+ Misses        192     124       -68

todo · 2019-06-15T03:31:56Z

once id is re-added to NotebookSession's attributes readd this

bookstore/bookstore/client/tests/test_nb_client.py

Lines 93 to 98 in 1c7a9b7

    
           # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this 
        
           assert notebook_session.path == session_dict["path"] 
        
           assert notebook_session.name == session_dict["name"] 
        
           assert notebook_session.type == session_dict['type'] 
        
           assert notebook_session.kernel == KernelInfo(**session_dict["kernel"]) 
        
           assert notebook_session.notebook == session_dict["notebook"]

This comment was generated by todo based on a `TODO` comment in `1c7a9b7` in #136. cc @mpacer.

mpacer · 2019-06-15T03:39:02Z

There's always something super disappointing when you see a code coverage report for a testing PR that reports a drop in test coverage for some reason…

I don't get it — the tests get picked up by pytest-cov locally:

---------- coverage: platform darwin, python 3.6.8-final-0 -----------
Name                               Stmts   Miss  Cover
------------------------------------------------------
bookstore/__init__.py                 10      1    90%
bookstore/_version.py                 10      0   100%
bookstore/archive.py                  49     11    78%
bookstore/bookstore_config.py         17      0   100%
bookstore/client/__init__.py           1      0   100%
bookstore/client/nb_client.py        112     25    78%
bookstore/client/store_client.py      26     18    31%
bookstore/clone.py                    82     18    78%
bookstore/handlers.py                 28     14    50%
bookstore/publish.py                  38     25    34%
bookstore/s3_paths.py                  9      0   100%
bookstore/utils.py                    12      0   100%
bookstore/validation.py               12     12     0%
------------------------------------------------------
TOTAL                                406    124    69%

todo · 2019-06-15T03:47:49Z

once id is re-added to NotebookSession's attributes readd this

bookstore/bookstore/client/tests/test_nb_client.py

Lines 93 to 98 in 0bf64ab

    
           # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this 
        
           assert notebook_session.path == session_dict["path"] 
        
           assert notebook_session.name == session_dict["name"] 
        
           assert notebook_session.type == session_dict['type'] 
        
           assert notebook_session.kernel == KernelInfo(**session_dict["kernel"]) 
        
           assert notebook_session.notebook == session_dict["notebook"]

This comment was generated by todo based on a `TODO` comment in `0bf64ab` in #136. cc @mpacer.

willingc

@mpacer the files are pretty small so I think it is fine to leave type and client together for now.

I would move the test files under the bookstore/tests/client directory instead of keeping with client. It keeps the tests together.

It tends to be more common in tests to have assert actual expected pattern though the opposite is technically valid. It's slightly more readable to do the first. For example:

assert extract_kernel_id(connection_file) == expected_kernel_id

would read from left to right as: If I test the extract_kernel_id function with a connection file, I would expect to receive this expected kernel id. It also makes it a bit easier for the reviewer to scan down the left of the page to see the commands executed.

willingc · 2019-06-15T14:44:29Z

bookstore/client/tests/test_nb_client.py

@@ -0,0 +1,116 @@
+import pytest


Consider moving the client tests to bookstore/tests/client/test_nb_client.py so all tests are kept together.

Sounds good

bookstore/client/nb_client.py

willingc · 2019-06-15T14:49:18Z

bookstore/client/nb_client.py

@@ -78,16 +78,29 @@ def __init__(self, *args, id, name, last_activity, execution_state, connections)
    def __repr__(self):
        return json.dumps(self.model, indent=2)

+    def __eq__(self, other):
+        if isinstance(other, KernelInfo):


Good job checking the object is a KernelInfo instance.

bookstore/client/nb_client.py

willingc · 2019-06-15T14:58:05Z

bookstore/client/tests/test_nb_client.py

+)
+
+
+@pytest.fixture


Consider moving all fixtures into their own file and importing that file. This makes the tests more visible and the test setup more reusable in other tests.

Sounds good

willingc · 2019-06-15T15:00:16Z

bookstore/client/tests/test_nb_client.py

+
+
+def test_notebook_session_class(notebook_session, session_dict):
+    # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this


Let's remove the comments in this file and the other and instead open an issue to track.

I created #135

willingc · 2019-06-15T15:01:41Z

bookstore/client/tests/test_nb_client.py

+    ],
+)
+def test_extract_kernel_id(connection_file, expected_kernel_id):
+    assert expected_kernel_id == extract_kernel_id(connection_file)


Perhaps flip the assert statement. assert <test_result> <expected result>

Sounds good

willingc · 2019-06-15T15:02:33Z

tox.ini

@@ -73,7 +73,7 @@ deps =
 depends =
    py36: clean
 commands =
-    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always bookstore/tests/
+    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always


I would be inclined to leave the directory here and move the client tests to bookstore/tests/client/...

willingc · 2019-06-15T15:03:21Z

tox.ini

@@ -73,7 +73,7 @@ deps =
 depends =
    py36: clean
 commands =
-    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always bookstore/tests/
+    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always


I would be inclined to leave the directory here and move the client tests to bookstore/tests/client/...

willingc · 2019-06-15T15:03:42Z

tox.ini

@@ -73,7 +73,7 @@ deps =
 depends =
    py36: clean
 commands =
-    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always bookstore/tests/
+    pytest -v --maxfail=2 --cov-config=.coveragerc --cov=bookstore -W always


I would be inclined to leave the directory here and move the client tests to bookstore/tests/client/...

todo · 2019-06-24T18:38:29Z

once id is re-added to NotebookSession's attributes readd this

bookstore/bookstore/client/tests/test_nb_client.py

Lines 93 to 98 in f72a1bd

    
           # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this 
        
           assert notebook_session.path == session_dict["path"] 
        
           assert notebook_session.name == session_dict["name"] 
        
           assert notebook_session.type == session_dict['type'] 
        
           assert notebook_session.kernel == KernelInfo(**session_dict["kernel"]) 
        
           assert notebook_session.notebook == session_dict["notebook"]

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

todo · 2019-06-24T18:38:32Z

once id is re-added to NotebookSession's attributes readd this

bookstore/bookstore/client/tests/test_nb_client.py

Lines 93 to 98 in f72a1bd

    
           # assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this 
        
           assert notebook_session.path == session_dict["path"] 
        
           assert notebook_session.name == session_dict["name"] 
        
           assert notebook_session.type == session_dict['type'] 
        
           assert notebook_session.kernel == KernelInfo(**session_dict["kernel"]) 
        
           assert notebook_session.notebook == session_dict["notebook"]

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

- Also makes minor improvements to NotebookSession's deprecated notebook by providing a default argument in the case that the field is removed.

Co-Authored-By: Carol Willing <[email protected]>

mpacer · 2019-06-24T19:41:17Z

Thank you for the reviews @willingc! I think I addressed them all, please let me know if not.

willingc · 2019-06-24T20:16:09Z

Fantastic @mpacer. Happy to merge.

mpacer requested a review from willingc June 15, 2019 03:01

mpacer force-pushed the nb_client_test branch from 60249b2 to 1c7a9b7 Compare June 15, 2019 03:31

willingc reviewed Jun 15, 2019

View reviewed changes

mpacer and others added 9 commits June 24, 2019 11:38

Improve KernelInfo with __eq__

324e177

- Also makes minor improvements to NotebookSession's deprecated notebook by providing a default argument in the case that the field is removed.

Test basic functionality for KernelInfo init and attribute access

e5c1bfa

Test basic functionality for NotebookSession init and attribute access

21e3be7

Add further identity test improvements to NotebookSession and KernelInfo

d60509d

Add tests for extract_kernel_id; includes previously problematic example

cc3a14b

Test LiveNotebookRecord init, attribute access, & equality

ba9f845

Apply suggestions from code review

90c7c18

Co-Authored-By: Carol Willing <[email protected]>

moved client tests to tests/client/ per review suggestions

bbf64ab

Move fixtures into their own module; complete review suggestions

74d15da

mpacer force-pushed the nb_client_test branch from f72a1bd to 74d15da Compare June 24, 2019 19:40

willingc merged commit 57f46e8 into nteract:master Jun 24, 2019

mpacer added this to the 2.3 milestone Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test nb_client module #136

Test nb_client module #136

mpacer commented Jun 15, 2019

todo bot commented Jun 15, 2019

codecov bot commented Jun 15, 2019

codecov bot commented Jun 15, 2019 •

edited

Loading

todo bot commented Jun 15, 2019

mpacer commented Jun 15, 2019

todo bot commented Jun 15, 2019

willingc left a comment

willingc Jun 15, 2019

mpacer Jun 24, 2019

willingc Jun 15, 2019

mpacer Jun 24, 2019

willingc Jun 15, 2019

mpacer Jun 24, 2019

willingc Jun 15, 2019

mpacer Jun 24, 2019 •

edited

Loading

willingc Jun 15, 2019

mpacer Jun 24, 2019

willingc Jun 15, 2019

mpacer Jun 24, 2019

willingc Jun 15, 2019

willingc Jun 15, 2019

todo bot commented Jun 24, 2019

todo bot commented Jun 24, 2019

mpacer commented Jun 24, 2019

willingc commented Jun 24, 2019



		def test_notebook_session_class(notebook_session, session_dict):
		# assert notebook_session.id == session_dict['id'] # TODO: once id is re-added to NotebookSession's attributes readd this

		)


		@pytest.fixture

Test nb_client module #136

Test nb_client module #136

Conversation

mpacer commented Jun 15, 2019

todo bot commented Jun 15, 2019

once id is re-added to NotebookSession's attributes readd this

This comment was generated by todo based on a TODO comment in 60249b2 in #136. cc @mpacer.

codecov bot commented Jun 15, 2019

Codecov Report

codecov bot commented Jun 15, 2019 • edited Loading

Codecov Report

todo bot commented Jun 15, 2019

once id is re-added to NotebookSession's attributes readd this

This comment was generated by todo based on a TODO comment in 1c7a9b7 in #136. cc @mpacer.

mpacer commented Jun 15, 2019

todo bot commented Jun 15, 2019

once id is re-added to NotebookSession's attributes readd this

This comment was generated by todo based on a TODO comment in 0bf64ab in #136. cc @mpacer.

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpacer Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

todo bot commented Jun 24, 2019

once id is re-added to NotebookSession's attributes readd this

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

todo bot commented Jun 24, 2019

once id is re-added to NotebookSession's attributes readd this

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

mpacer commented Jun 24, 2019

willingc commented Jun 24, 2019

This comment was generated by todo based on a `TODO` comment in `60249b2` in #136. cc @mpacer.

codecov bot commented Jun 15, 2019 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `1c7a9b7` in #136. cc @mpacer.

This comment was generated by todo based on a `TODO` comment in `0bf64ab` in #136. cc @mpacer.

mpacer Jun 24, 2019 •

edited

Loading

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

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