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

More unit tests #8

Merged
merged 16 commits into from
Feb 10, 2015
Merged

More unit tests #8

merged 16 commits into from
Feb 10, 2015

Conversation

payne92
Copy link
Contributor

@payne92 payne92 commented Feb 6, 2015

Added methods to read all pending message and all pending stdout

Added simple test case for a shell command (whoami)

Tests run on all terminal manager types

Test case for named terminal manager with no name provided

@tornado.testing.gen_test
def test_basic_command(self):
for url in self.test_urls:
tm = yield self.get_term_client('/named/foo')
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be passed url instead of '/named/foo'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


@tornado.testing.gen_test
def test_max_terminals(self):
urls = ["/named/%d" % i for i in range(MAX_TERMS+1)]
Copy link
Member

Choose a reason for hiding this comment

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

Since this is probably going to be relatively slow, let's skip this test until there's a way of checking that the final one isn't created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you consider slow? This whole case finishes in a few seconds, at most. I could drop MAX_TERMs to 2 or 3?

Copy link
Member

Choose a reason for hiding this comment

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

I just mean that since this test doesn't currently create the (N+1)th terminal, it's not really checking anything that isn't checked elsewhere, so we may as well leave it out until we work out how to test what it aims to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean. See the lines below both max_terminals tests (FIXME): the MAX_TERM+1 case is commented out because of bugs (e.g. #9).

If this PR otherwise looks good to you, after you accept it, I'll send modular individual PRs (e.g. unit test case demonstrating the bug + the fix) for these other issues.

Copy link
Member

Choose a reason for hiding this comment

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

OK, great. Yes, I think this otherwise looks good - thanks!

def test_max_terminals(self):
tms = yield self.get_term_clients(['/unique'] * MAX_TERMS)
pids = yield self.get_pids(tms)
self.assertEqual(len(set(pids)), MAX_TERMS) # All PIDs the same
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

takluyver added a commit that referenced this pull request Feb 10, 2015
@takluyver takluyver merged commit 5f021b9 into jupyter:master Feb 10, 2015
@takluyver
Copy link
Member

Hmm, after I merged this, there were some test failures: https://travis-ci.org/takluyver/terminado/builds/50237451 Can you take a look?

@payne92
Copy link
Contributor Author

payne92 commented Feb 10, 2015

There is some racing happening on the timeouts. The Travis environment seems especially sensitive to time variances.

Can you kick it off again? In the meantime, I will widen some of the timeouts.

-andy

On Feb 10, 2015, at 1:06 PM, Thomas Kluyver [email protected] wrote:

Hmm, after I merged this, there were some test failures: https://travis-ci.org/takluyver/terminado/builds/50237451 Can you take a look?


Reply to this email directly or view it on GitHub.

@payne92
Copy link
Contributor Author

payne92 commented Feb 10, 2015

More liberal read timeouts: #10

Sorry for the noise on this one.

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.

2 participants