-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Test Coverage for IPython #1256
Conversation
evalml/utils/gen_utils.py
Outdated
return True | ||
return False | ||
except ImportError: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for except ImportError
if you already have except Exception
Codecov Report
@@ Coverage Diff @@
## main #1256 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 207 207
Lines 13142 13157 +15
=======================================
+ Hits 13133 13149 +16
+ Misses 9 8 -1
Continue to review full report at Codecov.
|
test-requirements.txt
Outdated
@@ -2,6 +2,7 @@ pytest==4.4.1 | |||
pytest-xdist==1.26.1 | |||
pytest-cov==2.6.1 | |||
nbval==0.9.3 | |||
IPython>=3.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to test-requirements.txt
rather than requirements.txt
since nbval
has an IPython>=5.0.0
dependency, causing the core-dependencies=True
test to fail (the full dependency is ipython -> ipykernel -> nbval
). @dsherry preferred it to be in requirements.txt
, but this change made the tests pass. Input on this is welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! Since nbval
requires IPython
I do agree on the move of IPython
to the requirements. Good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bchen1116 This looks good to me!
@patch('evalml.utils.gen_utils.import_or_raise') | ||
def test_jupyter_check(mock_import_or_raise): | ||
mock_import_or_raise.return_value = MagicMock() | ||
mock_import_or_raise().core.getipython.get_ipython.return_value = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
fix #1187