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

[READY TO MERGE] async_poll_for and not poll_for inside a @gentest test #607

Merged
merged 3 commits into from Oct 25, 2013
Merged

[READY TO MERGE] async_poll_for and not poll_for inside a @gentest test #607

merged 3 commits into from Oct 25, 2013

Conversation

thefab
Copy link
Contributor

@thefab thefab commented Oct 25, 2013

Because of discusion on #605

Ready to merge (now)

@thefab thefab mentioned this pull request Oct 25, 2013
but commiting the rename was not intentional
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 52f8083 on thefab:again_some_random_test_fails into 36a6098 on mozilla-services:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0802f1f on thefab:again_some_random_test_fails into 36a6098 on mozilla-services:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4eade0a on thefab:again_some_random_test_fails into 36a6098 on mozilla-services:master.

@scottkmaxwell
Copy link
Contributor

Oh good. I've been noticing a bunch of those "arbiter is already running arbiter_reload_config command". Is that what you fixed?

@tarekziade
Copy link
Member

I think @thefab just fixed all the remaining async issues we had in the tests. wOOt :)

let me know when we should merge

@scottkmaxwell
Copy link
Contributor

I still never got the flapping plug-in working. Once this is merged in, I'll test again. Seemed like it might be an async issue.

@tarekziade
Copy link
Member

@scottkmaxwell ok thanks. We might have a regression there from the async switch nonetheless - if it's the case we'll deal with it on its side. Will wait for @thefab green light to merge this one.

@thefab
Copy link
Contributor Author

thefab commented Oct 25, 2013

There are still some random problems on Travis (on random python versions):

This one is about send_signal_child(), some recent changes here, no ?
https://travis-ci.org/mozilla-services/circus/jobs/13060612#L455

This one is maybe because of travis nodes are too slow (5 seconds default timeout of gen_test decorator):
https://travis-ci.org/mozilla-services/circus/jobs/13060614#L380

But the master is better with this pull request merged. So let's merge it and let's continue !

tarekziade added a commit that referenced this pull request Oct 25, 2013
[READY TO MERGE] async_poll_for and not poll_for inside a @gentest test
@tarekziade tarekziade merged commit 26b9f8e into circus-tent:master Oct 25, 2013
@tarekziade
Copy link
Member

one major fix is that we don't have anymore builds dying because of the POLL errors anymore so that's definitely looking way better

@scottkmaxwell
Copy link
Contributor

I have some changes to the signal system in #603 but that isn't merged in here yet. But I think we should catch that KeyError and transform it into a psutil.NoSuchProcess instead. I can add that to #603 if you like.

@tarekziade
Copy link
Member

@scottkmaxwell sure yeah. we also need more coverage there as I mentioned. Because if we're not catching those errors in the tests that's pretty bad

@scottkmaxwell
Copy link
Contributor

Yes, we need a whole bunch of signal tests. Can you point me to a good example of kicking off a custom process so I can do signal and stop tests?

@tarekziade
Copy link
Member

@scottkmaxwell you can use start_arbiter() and point it to a callable you want to run as a process in circusd. See for instance https://github.com/mozilla-services/circus/blob/master/circus/tests/test_stats_client.py#L48 that uses https://github.com/mozilla-services/circus/blob/master/circus/tests/test_stats_client.py#L14

In your callable you can loop indefinitely and interact with signals I guess

@scottkmaxwell
Copy link
Contributor

It will be a bit complex because I need to test interaction with the callable and its child processes. I'll give it a try.

@scottkmaxwell
Copy link
Contributor

OK, I added some signal tests. They are all in one test because they are serial, but I test sending a signal to a process, children only and recursive, and I test shutting down the process only and having the daemon children shutdown. It's all on #603 now.

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

4 participants