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

relax timeouts in terminal console and tests #3817

Merged
merged 5 commits into from Jul 29, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented Jul 28, 2013

avoids failures on slow kernels (especially on test VMs)

uses kernel_info to check if the kernel is alive, rather than executions.

and use kernel_info request to check instead of executions
should decrease false failures on slow VMs (travis)
@Carreau
Copy link
Member

Carreau commented Jul 28, 2013

+1

@ivanov
Copy link
Member

ivanov commented Jul 29, 2013

looks ok to me, but I'm in general opposed to having any kind of magic number naked in source code. With the exception of 0 and 1, it's preferable to have named constants. I don't know what (if any) the relationship between the numbers currently being used, or if they're just ad-hoc, and if it's at all possible to make them all one so that we could just set them in one place as opposed to search-replacing in a few places)

@minrk
Copy link
Member Author

minrk commented Jul 29, 2013

good point - made it Configurable, since that's what those are for.

@minrk
Copy link
Member Author

minrk commented Jul 29, 2013

and centralized the timeout values in the tests. There is a chance this PR will resolve false-positive failures in Travis, though I have said that before.

@Carreau
Copy link
Member

Carreau commented Jul 29, 2013

There is a chance this PR will resolve false-positive failures in Travis, though I have said that before.

There is a probability distribution that this PR get merged after time t > 0. I would suspect that the cumulative distribution tends to 1 when t go toward infinity.

@ivanov
Copy link
Member

ivanov commented Jul 29, 2013

@Carreau: I think you meant to say there's not a probability distribution that this PR doesn't get merged as t goes to infinity (testing and checking this updated PR now)

@Carreau
Copy link
Member

Carreau commented Jul 29, 2013

@ivanov I didn't wanted not to say it. Don't be angry.

@minrk
Copy link
Member Author

minrk commented Jul 29, 2013

So angry.

Carreau added a commit that referenced this pull request Jul 29, 2013
Relax timeouts in terminal console and tests

avoids failures on slow kernels (especially on test VMs)

uses kernel_info to check if the kernel is alive, rather than executions.
@Carreau Carreau merged commit 730a82e into ipython:master Jul 29, 2013
@Carreau
Copy link
Member

Carreau commented Jul 29, 2013

Those failure are annoying me a lot.
Merging.

# heart failed
if timeout is not None and (time.time() - tic) > timeout:
return False
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

there's no way to get to this line, if I'm following the logic properly. Is that right? (if so, remove it?)

@ivanov
Copy link
Member

ivanov commented Jul 29, 2013

oops, race condition with @Carreau, who's trigger happy with the merge button ;)

@minrk minrk deleted the console_test branch July 29, 2013 22:58
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Relax timeouts in terminal console and tests

avoids failures on slow kernels (especially on test VMs)

uses kernel_info to check if the kernel is alive, rather than executions.
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