Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

More tests to IOLoop #571

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

igorsobreira commented Jul 23, 2012

Hello,

I'm creating tests to ioloop.py module, I've created some tests for the creation methods (init, instance(), install()).

I plan to keep adding tests to reach a good coverage level. I'm sending this pull request now to fetch feedback as soon as possible. But I'm working basically on test coverage on my fork.

(there is also one test for httputil.py that I should have sent on my previous pull request some time ago)

Thanks,
Igor.

Owner

bdarnell commented Jul 31, 2012

Don't blindly delete IOLoop._instance in setUp. Instead, assert that the IOLoop is not initialized in setUp, and delete _instance in tearDown. This ensures that you don't leave an instance around when later tests are running.

Use assertTrue(a is b) instead of assertEqual(a, b) when checking that you've got the same IOLoop. It's unlikely that we'd add a non-identity comparison method to IOLoop, but using "is" makes the intention clearer. (in 2.7 and above we could use assertSame).

It looks like you're only ever using one "reverter" at a time, so it might be simpler to just do a try/finally in each test that needs it rather than pulling it out into a more generic mechanism.

"with assertRaises" only works in python 2.7+.

The instance lock test is complex and fragile, and I'm not sure it's really testing anything useful. I'd probably just get rid of it.

The last two tests don't have anything to do with the _instance singleton, so I'd put them in TestIOLoop instead of TestIOLoopCreation. But I'm concerned about fragile mocking and dependence on implementation details here too. I don't think the register handler test is necessary - what matters is that add_callback can wake up an idle IOLoop from another thread. Current implementations do that by registering a handler, but that need not be the case. I'd rather test this by making a multi-threaded version of test_add_callback_wakeup.

For CLOEXEC, the test needs to be conditional so it doesn't break on platforms that don't have CLOEXEC (i.e. windows). There's no need to mock the poller - on platforms where the native poller doesn't have a fileno, it doesn't matter. So just check ioloop._impl.fileno(), and if there is one test its CLOEXEC flag (but then this is a pretty weak test - what we really want is to ensure that every fd created by the IOLoop has CLOEXEC, but that's a really annoying thing to try and test for)

Contributor

igorsobreira commented Jul 31, 2012

Thanks for the review! I'll work on these issues.

The idea with TestIOLoopCreation was to test the creation of the ioloop itself, and also what happens when it's created. That's why the last two tests are there. But I can move to TestIOLoop

I get your point on add_handler. A multi-threaded test for add_callback really makes more sense.

Good point on CLOEXEC. ioloop._impl.fileno() is a good conditional test indeed. I'll try to find a solution to test for CLOEXEC without mocking IOLoop.

@bdarnell bdarnell added the ioloop label Jul 16, 2014

@bdarnell bdarnell closed this Jul 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment