Skip to content
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

Don't redirect stdout if nose machinery is not present #427

Conversation

edisongustavo
Copy link
Contributor

@edisongustavo edisongustavo commented Sep 2, 2019

By spawning subprocesses with stdout = open('/dev/null') then PyCharm
is not able to attach to them.

This is specially painful when debugging the tests in test_kernel(),
where the methods kernel() do spawn kernels in subprocesses if
required.

I've recorded a quick video showing this in action: https://youtu.be/GfdADoikFzg

More thoughts

I'm not entirely sure on how all of this test machinery behaves, but it doesn't look like nose is being used anymore as the framework of tests.

This can be seen in the .travis.yml and the fact that the symbol iptest_stdstreams_fileno which is defined in https://github.com/ipython/ipython/blob/e1b53e9ef91a43b9e275bb9e48b4253218375d87/IPython/testing/iptest.py#L337 is a method, and its usage in new_kernel() was obtaining a reference to it and not invoking it, which is probably a mistake.

By spawning subprocesses with `stdout = open('/dev/null')` then PyCharm
is not able to attach to them.

This is specially painful when debugging the tests in test_kernel(),
where the methods `kernel()` do spawn kernels in subprocesses if
required.
@blink1073
Copy link
Member

blink1073 commented Sep 12, 2019

We're still using nose as a library but not as the test runner. Thanks for cleaning this up!

@blink1073 blink1073 merged commit 97d10d4 into ipython:master Sep 12, 2019
@edisongustavo
Copy link
Contributor Author

edisongustavo commented Sep 12, 2019

Thanks a lot for merging!

We're still using nose as a library but not as the test runner. Thanks for cleaning this up!

Yes, I saw that.

Would you like a PR cleaning that up?

@blink1073
Copy link
Member

Yes, please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants