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

Twilio Enqueue method #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Twilio Enqueue method #5

wants to merge 4 commits into from

Conversation

tmlee
Copy link

@tmlee tmlee commented Mar 26, 2013

We found the need to test the Enqueue method. It tests the match for waitUrl, action, and noun.
Code and spec attached. let me know what you think?

@JMongol
Copy link
Owner

JMongol commented Mar 26, 2013

Thanks for contributing this. A few thoughts for you.

First, a minor stylistic comment - the has_enqueue? method is a little dense for my tastes, and I'd like to see it easier to parse and understand for future maintainers. Can you work on that? Maybe even just a few comments explaining what's happening would be helpful.

Besides that, it seems that in order to test a complete scenario around Enqueue, you also need to trigger a dequeue. For instance, a full scenario might be to create a couple of Enqueues, then cause a dequeue to be triggered, and validate that the call was transferred or some other behavior occurred. I'm not super familiar with Enqueue, but I'm assuming you must be, so please think through this scenario as it would apply end-to-end in a real app. One of the major design goals of TTT is to enable this type of end-to-end testing, so before we take a new feature, I'd like to make sure we can do that for the new feature.

On that note, it might also be worth creating such an end-to-end test that demonstrates , not only to validate that TTT is working properly, but to serve as a tutorial for those that want to write a similar test.

Jack

@tmlee
Copy link
Author

tmlee commented Mar 27, 2013

Hey Jack, thanks for the insight. its always better that way since you know Twilio & TTT better.

Agree that the has_enqueue is a lil dense, the Twilio's seems to have a few key attributes that are worth testing. waitUrl which tells where to point to while waiting in the queue, noun which will create the new queue with that name, and possibly action.
Feel that the better way to check them, is to split the method like the way you did it for .

Just happen to start taking a peek at Twilio; I sure will stumble upon that sooner or later, since now am looking to test a phone tree and i imagine this would suffice for now before digging down to that.

@JMongol
Copy link
Owner

JMongol commented Mar 27, 2013

Please feel free to submit an updated pull request when you've had a chance to digest the dequeue part of it and write some tests for that. I don't think we can merge this until then, but what you have is definitely a good start.

Two possible implementation paths for that which cross my mind:

  1. Adding some sort of simple "dequeue-call" utility method to TTT somewhere.
  2. Providing a mock for the API in the twilio-ruby gem that deals with the dequeue process.

(1) is obviously the simpler case. (2) has some interesting advantages and offers some unique possibilities, but is of course more work. Again, I'm not that familiar with Enqueue, so perhaps you (or someone else) with more intimate knowledge of Enqueue can provide your thoughts.

Jack

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