Add unit tests + CI + code coverage #10
Conversation
Adapted from https://github.com/jupyterhub/binderhub/blob/master/binderhub/tests/test_eventlog.py, which is where a lot of the code comes from
All are pure python libraries with minimal other dependencies. We should probably remove 'notebook' from here, and move it to its own jupyter_telemetry_serverextension package.
Makes counting coverage much easier. Plus we do not ship these with our package.
This should be added into the notebook package itself. There's a WIP PR here: jupyter/notebook#4752 This also lets us remove 'notebook' as a dependency, and use python 3.5 as our minimum version
Much cleaner than tempfile
|
jupyter/notebook#4752 is a companion PR in the notebook repo that depends on this PR. It so far implements just enough scaffolding to have a test event schema for all accesses via the contents manager class. I've moed the API endpoint code from this repo over there as well. |
|
|
||
|
|
||
| def test_register_invalid(): | ||
| """ |
There was a problem hiding this comment.
I think it would be worth adding comments that describe the specific "failures" we are testing in each chunk below.
Another approach would be to break each chunk into a separate unit test. I doesn't matter to me, but some of these failures aren't what I expect. See comments below.
|
|
||
| with pytest.raises(ValueError): | ||
| el.register_schema({ | ||
| 'properties': {} |
There was a problem hiding this comment.
Should this throw a KeyError instead? The problem is caused by missing keys (i.e. $id and $version), but I can't decide if that should result in a ValueError or KeyError.
There was a problem hiding this comment.
KeyError says 'Raised when a mapping (dictionary) key is not found in the set of existing keys.', while ValueError says 'Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.'
This makes me feel ValueError is the right thing, since we aren't passing anything in directly with the key.
| 'properties': {} | ||
| }) | ||
|
|
||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
It's not immediately obvious to me why this should fail. I certainly didn't notice that the failure is caused by an extra $ before version. Maybe add a comment to this source code saying
# testing invalid key: "$version" when it should be "version".
tests/test_register_schema.py
Outdated
| el.register_schema({ | ||
| '$id': 'something', | ||
| '$version': 1, | ||
| 'properties': { |
There was a problem hiding this comment.
The value of properties is excessive since we're just testing the failure of the $version key.
| } | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Should we add a test that fails because a dunder key of properties is set in the given schema?
| assert '__timestamp__' in event_capsule | ||
| # Remove timestamp from capsule when checking equality, since it is gonna vary | ||
| del event_capsule['__timestamp__'] | ||
| assert event_capsule == { |
There was a problem hiding this comment.
This isn't guaranteed to work in Python 2, but we don't have to worry about that, right? ;)
There was a problem hiding this comment.
yeah, let's not worry about that for right now :) We can later if we wanna
|
Thanks for the comments, @Zsailer! I've addressed them all I think |
| @@ -14,7 +14,7 @@ | |||
| license = 'BSD', | |||
| platforms = "Linux, Mac OS X, Windows", | |||
| keywords = ['Jupyter', 'JupyterLab'], | |||
| python_requires = '>=3.6', | |||
| python_requires = '>=3.5', | |||
There was a problem hiding this comment.
I've come across this on another Git extension as well - Is there somewhere Jupyter mandates Py3.5 compatibility?
There was a problem hiding this comment.
I added it to make jupyter/notebook#4752 pass, since notebook's tests seem to require a minimum of python 3.5.
| '$version': 1, # This should been 'version' | ||
| }) | ||
|
|
||
| def test_reserved_properties(): |
A lot of the code also came from there :)