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

Basic exercise of ipython [subcommand] -h and help-all #4125

Merged
merged 6 commits into from Sep 4, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented Aug 28, 2013

for various subcommands.

Prompted by the fact that ip qtconsole -h and ip nbconvert -h are both broken in master.
This PR also fixes both.

Should be backported to 1.1

@takluyver
Copy link
Member

The decorators seem a bit on the magical side given the need for them, but that's a matter of preference. Other than that, +1.

@minrk
Copy link
Member Author

minrk commented Aug 28, 2013

I agree - I just didn't want to do a huge amount of copy/paste in case anything should change. Do you have a better way to generate a bunch of tests that differ by a simple string key?

@takluyver
Copy link
Member

I think I'd just use tt.help_output_test('profile') etc. and not bother with the docstrings.

@minrk
Copy link
Member Author

minrk commented Aug 28, 2013

decorators removed, tests generated now from a text-editor macro instead of at runtime.

@takluyver
Copy link
Member

LGTM

it has `config=True` traits

Currently in master `ipython console -h` will fail because of this.
It doesn't affect 1.0, I'm not sure why.
should help catch when we break these things.
prevented `nbconvert -h` from working
help-all is a superset of `-h`, so no need to do both,
since these tests are slow.
@ivanov
Copy link
Member

ivanov commented Aug 30, 2013

I expressed concern in person that this slows down our test suite. In particular, here are the running times on my laptop with an SSD for this PR: Ran 14 test groups in 172.081s

whereas master on cold cache, and warmed up runs in about 90 seconds.

This may or may not be worth updating, since it'll become less of an issue when we re-factor and parallelize the test suite. We also currently have no way of marking tests as "slow" and to skip them unless the --all flag is passed.

(these times were before @minrk updated to only run --help-all, instead of `--help'

@ivanov
Copy link
Member

ivanov commented Aug 30, 2013

actually, an update to my earlier comment - i hadn't updated IPython master on my laptop for a week, and ipython master test suite now runs in 160 seconds :( Min's looking into this right now

@minrk
Copy link
Member Author

minrk commented Aug 30, 2013

Almost all of the new slowness is in new kernel tests introduced in the last week or so, which spin up new kernels, some more than necessary (it can take up to three seconds to start and stop a kernel subprocess), not this PR. I will investigate reusing kernel instances for more tests, which should help. But that's for another PR.

@minrk
Copy link
Member Author

minrk commented Sep 4, 2013

merging soon unless objections - traceback on nbconvert help output is bugging me.

@ivanov
Copy link
Member

ivanov commented Sep 4, 2013

lemme just test it locally one more time

@minrk
Copy link
Member Author

minrk commented Sep 4, 2013

I'm doing the same

@ivanov
Copy link
Member

ivanov commented Sep 4, 2013

looks good, i haven't pushed the green button for a while, so I'm gonna go for it!

ivanov added a commit that referenced this pull request Sep 4, 2013
Basic exercise of `ipython [subcommand] -h` and help-all
@ivanov ivanov merged commit cf7fad3 into ipython:master Sep 4, 2013
@ivanov
Copy link
Member

ivanov commented Sep 4, 2013

lemme see if the backport script still works, now...

@minrk
Copy link
Member Author

minrk commented Sep 4, 2013

will need to be careful with the backport, because the Transformer/Preprocessor change will want to be excluded.

@minrk minrk deleted the test-help-output branch September 4, 2013 19:17
@ivanov
Copy link
Member

ivanov commented Sep 4, 2013

hmm, so an hour later, can I ask you for an assist, @minrk on the backport? I've edited the patch down to just two error which don't apply, but don't know how to proceed :\

error: patch failed: IPython/kernel/tests/test_kernel.py:250
error: patch failed: IPython/nbconvert/nbconvertapp.py:87

@minrk
Copy link
Member Author

minrk commented Sep 4, 2013

sure, I'll look at it.

@minrk
Copy link
Member Author

minrk commented Sep 4, 2013

worked with this patch

minrk added a commit that referenced this pull request Sep 4, 2013
…lp-all

for various subcommands.

Prompted by the fact that `ip qtconsole -h` and `ip nbconvert -h` are both broken in master.
This PR also fixes both.

Should be backported to 1.1
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Basic exercise of `ipython [subcommand] -h` and help-all
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

3 participants