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

Some gardening on iptest result reporting #5326

Merged
merged 6 commits into from
Mar 20, 2014

Conversation

takluyver
Copy link
Member

My nice concise output when running iptest -j6 was being messed up by some lines like "Running js/tree tests in directory: '/tmp/tmp994_q5'" that had snuck in. There wasn't a good way for the test process to add potentially useful information on failure without it showing up in the concise view on success. This adds that: a TestController.dump_failure() method which prints stdout from the test process. JSController overrides this to add the notebook directory.

While looking at this, @ivanov pointed out that "IPython test group" was unnecessary, so I shortened it to "Test group".

@ivanov
Copy link
Member

ivanov commented Mar 10, 2014

Are we planting 🍆, 🍅 or 🌽?

One minor thing: right now it doesn't actually print the directory name:
Ran tests with notebook directory <IPython.utils.tempdir.TemporaryDirectory object at 0x7fb597fc40d0>

@@ -208,6 +216,10 @@ def _init_server(self):
self.server.start()
self.server_port = q.get()

def dump_failure(self):
print("Ran tests with notebook directory %r" % self.nbdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.nbdir.name

@ivanov
Copy link
Member

ivanov commented Mar 10, 2014

Do we need to modify something in the if options.fast == 1: branch, since dump won't get called there right now.

@takluyver
Copy link
Member Author

Fixed printing the directory name.

I'm not exactly sure what to do for the fast == 1 branch, because we're not buffering stdout there, so calling dump_failure() doesn't make sense. Maybe we need to rework the design a bit...

Add a setup() method to be called when we know we're going to use a test
group, for creating temporary dirs etc., and a print_extra_info() method
to display extra information.
@takluyver
Copy link
Member Author

OK, this got a little more complicated. Instead of dump_failure(), there is now print_extra_info(). If we're running with a -j or --fast option, this is called on failure before we dump the output. Otherwise, it's called before starting the test run.

However, that method may need info such as the locations of temporary directories. In fact, that's the motivation for adding it in the first place. Those can't be defined in launch(), because when we're not running in parallel, it's called before launch(). And they shouldn't be defined in __init__(), because that's called even for test groups that don't get run. So there's a new setup() method, called before the tests are run, but only when we know we will try to run them, in which temporary directories etc. should be created. The JS tests also use this to start the notebook server.

@ellisonbg
Copy link
Member

There are two more "IPython test group" instances that can be shortened to "Test group".

@ellisonbg
Copy link
Member

While we are at it in this file, the command line help for the -j/--fast option sounds like it is a boolean option: "Run test sections in parallel.". Maybe something that clarifies this is an option to run the tests in parallel with this many processes: "Run test sections in parallel with this many processes" or something like that.

@ellisonbg
Copy link
Member

I like the overall design of the refactoring though. I think this is mostly ready to do after a few minor changes.

@takluyver
Copy link
Member Author

Well, the easiest use of -j is as a boolean flag - it will use the number of cores on your machine. I'll see if I can improve the docstring, though.

@ellisonbg
Copy link
Member

Ahh, more complicated (and nicer) than I though. Even more reason to clarify the help string. Thnx

@takluyver
Copy link
Member Author

OK, I've cleaned those things up.

@ellisonbg
Copy link
Member

Looks good, merging.

ellisonbg added a commit that referenced this pull request Mar 20, 2014
Some gardening on iptest result reporting
@ellisonbg ellisonbg merged commit fa75ebe into ipython:master Mar 20, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Some gardening on iptest result reporting
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.

3 participants