-
Notifications
You must be signed in to change notification settings - Fork 39
Retrieve and run app engine tasks from testbed queues #60
Conversation
The purpose is to run full local integration tests with the appengine testbed.
def _clear_context(): | ||
"""Clear a contenxt. This is not normally used, but is needed in testing | ||
when running asyncs within a single process. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good. I wrestled with this issues in my furious work. I wonder if we should put stuff like this in a separate test/utils module and keep it out of the regular code base since we're not going to use it in furious itself? Thoughts @beaulyddon-wf and @robertkluin-wf ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, might make sense. Certainly don't want somebody thinking that we do this or that they could do this on a "prod" run.
Make clear_context more robust by creating a new uninitialized state instead of removing known modifications. Also add a test for clear_context.
Others prefer a = a or b instead of a |= b for clarity, so update that. Add tests for the _execute_queue() function containing the change.
Python optimizes "or" statements so that the second part of the condition is not executed if the first part of the condition is True. Putting _execute_queue() first ensures that it always gets called. Added unittests for execute_queues.
If tasks have a modified os.environ, raise an exception. This is to help prevent tasks from interfering with each other.
Updated with changes that Robert suggested. Tanner has an unanswered question in an outdated diff about _clear_context's location. |
|
||
# Decode the body and process the task. | ||
body = base64.b64decode(task['body']) | ||
return_code, func_path = process_async_task(task['headers'], body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we need to check the return_code here and raise or something for 500s, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a pass at raising return codes:
We were verifying a user's async task did not leave os.environ in a modified state after each task was run. Since 3rd party libraries sometimes modify os.environ, this check isn't easily enforceable.
@@ -0,0 +1,67 @@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed the Apache header here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, the header is there now.
We just need a simple function for furious asyncs to call in the tests. Previously we were using MessageProcessor as a place holder for a simple function callback, which is misleading. Now we're using functions like ctime and strftime in its place.
The comments so far should be addressed. Just let me know if you think of anything else. |
I'll give this a 👍 |
Retrieve and run app engine tasks from testbed queues
Enables full local integration tests with the appengine testbed.
@robertkluin @beaulyddon-wf @johnlockwood-wf @tannermiller-wf