spyder-ide / spyder-terminal

Run system terminals inside Spyder. Works on Linux, macOS and Windows.
MIT License
230 stars 78 forks source link

test_terminal_resize assumes python command #179

Closed bnavigator closed 4 years ago

bnavigator commented 4 years ago

https://github.com/spyder-ide/spyder-terminal/blob/1797a86ee0f4e94627493b93440b5b99f15437a5/spyder_terminal/server/tests/test_server.py#L167

This line in test_terminal_resize() assumes there is a command in the path named python. This is not necessarily true for all installations. The packaging on OpenSUSE build service for example fails, because it only has python3 by default. On other systems it could still point to `python2

ccordoba12 commented 4 years ago

I disagree with adding an extra check for this. spyder-terminal depends on Spyder, which depends on Python. So Spyder necessarily needs to be installed (it's part of the package requirements).

bnavigator commented 4 years ago

I don't understand your comment. What has the dependency on spyder and python to do with this issue? It is about the correct executable of the call python. You cannot control the user's environment.

ccordoba12 commented 4 years ago

I don't understand your point either. python must be installed in the environment where the tests are running.

ccordoba12 commented 4 years ago

The packaging on OpenSUSE build service for example fails, because it only has python3 by default.

Ok, I didn't read this part (Linux and its quirkiness).

@steff456, please change the python used in that test to python3.

steff456 commented 4 years ago

Should we do this for the new release of spyder-terminal that doesn't support python 2?

bnavigator commented 4 years ago

It doesn't matter if you support python2 or not. What matters is, whether the test environment has the correct python in their PATH.

(As long as you only care about your own CIs being supported test environments and those are all set up with the correct python in their PATH, this issue report is moot)

ccordoba12 commented 4 years ago

Ok, then I think we should simply skip this test if we're not in our CIs. @steff456, this is an example of how you can do this:

https://github.com/spyder-ide/spyder/blob/31c2129d4629ff27bc3c36a4235f26a1e15fe0ce/spyder/app/tests/test_mainwindow.py#L241-L243

bnavigator commented 4 years ago

Ok, then I think we should simply skip this test if we're not in our CIs.

Please don't. I understand that you only want to support your own test environments, that's fine. But there is no need to actively shut out others. The test itself test_terminal_resize is not specific to the CI but is also useful for example to test if a package works fine. Distribution maintainers can adjust the python call to their environment if necessary, but most likely would prefer to run the test rather than skip it. Of course they can also revert the proposed skip, but why make it harder for them? Please don't waste your resources on working against each other, just ignore the bug report if you don't want to fix it in a more general way.

For the same reason I would prefer you don't remove the tests from the pypi package as discussed in #184, but of course it is your choice.

ccordoba12 commented 4 years ago

Please don't. I understand that you only want to support your own test environments, that's fine.

Ok, I have no problem with that either. But what's your proposed solution then? I'm sorry but if you don't give us a hand here, we'll have to skip it.

For the same reason I would prefer you don't remove the tests from the pypi package as discussed in #184, but of course it is your choice.

Sorry but that was our declared intention since we released this package (but we noticed it until now that we are distributing our tests). So that's not going to change.

bnavigator commented 4 years ago

But what's your proposed solution then?

Either "won't fix" or call the correct python flavor. I can sumbit a PR later, so you can just let this issue sit.

ccordoba12 commented 4 years ago

I can sumbit a PR later, so you can just let this issue sit.

Thanks!! That's greatly appreciated!