-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Start writing tests... #7
Conversation
@mwcraig , do we need |
astrowidgets/tests/test_api.py
Outdated
|
||
|
||
def test_load_fits(): | ||
image = ImageWidget() |
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.
My suggestion is to make these functions into a test class' methods. That way, you can initialize image = ImageWidget()
only once at setup_class()
and then use the same object throughout.
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.
Sounds reasonable; I'm going to keep implementing as functions for the moment so we can see whether there are any tests where we want separate ImageWidget
instances.
For example, it looks like setting cuts
with a string fails in a way users will find surprising if the data hasn't been set yet. In that case the cuts
are set to (0, 0)
(which makes sense) but that is probably not what the user wants; to test for this I think we want a new ImageWidget
without data for that test.
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.
Oh, I should have read this before commenting on #8
No, probably not; I just tried setting |
@drdavella , any advice on how to get rid of the |
It's possibly due to some regressions introduced in pytest 3.7, but it's hard for me to tell without the full test report. |
In that case, does the error go away if you use |
All of the tests currently pass 😀 so read to merge.... |
Unfortunately I'm using pytest 3.6.4 😢 I'll take another look in the morning... |
If you want to post the full log here I can take a look and see if anything obvious stands out. |
@mwcraig , now I am confused. Is the |
Yep, pytest failure is still a problem if doctest_plus is enabled. Full output from pytest is below. The main thing I don't understand is why pytest is digging down so deep into package dependencies to look for docstrings to test.
|
If I disable doctest_plus (by removing it from setup.cfg) the tests run successfully, so that's what I'll do later today, I think, unless we find a better fix before that... |
The only thing that from ginga.misc.log import get_logger
logger = get_logger('my_viewer', log_stderr=False,
log_file='ginga.log', level=40) Even then, I am not sure why it is choking on |
@mwcraig , 👍 to disabling doctest for now. Figuring out how to make it work, if needed, can be a separate issue. |
@mwcraig , I activated https://travis-ci.org/astropy/astrowidgets . FYI |
🎉 triggered the first build on travis! 🎈 |
The Sphinx failure is because |
Or maybe missing dependencies for Sphinx... 🤔 (nope, adding |
Is there any reason not to use conda-forge in the tests? That would pick up ipyevents as a conda package... |
I think I fixed the helpers problem....I had added it as |
Recent discussions suggested that conda is slower than pip. Otherwise I don't think any reason it won't work. |
Yeah, the conda solver has become really slow.... |
It passed! 🎉 |
Do you want me to squash before merging? |
I think @eteq and I have differing opinions on the topic of squashing... But I say yes. ;) |
I let GitHub do its squash and merge magic.... |
...right now these mostly fail, will work on in the morning.
There is one problem I'm hitting @pllim -- when I try
python setup.py test
locally I get an error when tests are being collected; any idea what the cause might be? In the meantime I'm running them withpython setup.py test --args '--continue-on-collection-errors'