Create a transparent proxy for test programs to use, with the

Create a transparent proxy for test programs to use, with the
ability to filter or manipulate content as it's passed through.

Create two example tests using this utility that cause
updates to close early to test resumes, and to add delays
to make sure the update still completes correctly.

What other tests should be created for this?

BUG=chromium-os:8207
TEST=Run by hand

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=8be02f9

[dgarrett, 2010-12-07 01:57:15]

Seems it was never published. Sigh.

[adlr, 2010-12-07 02:26:50]

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:137: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
80 cols

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:274: def __init__(self):
please add a docstring for each method/function

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:380: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
80 cols

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:460: port_flags = []
there seems to be code duplication here and in the next few functions. can the
common code be combined?

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:479: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
80 cols

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode45
lib/cros_test_proxy.py:45: def setup(self):
please add a docstring for each method/function

[sosa, 2010-12-07 03:44:36]

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:133: exception of code needed for ssh.
doc for proxy_port

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:268: """This filter causes the proxy to interrupte
the download 3 times
spelling

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:296: proxy_port = 8081
Comment about the default port choices

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:313: """Causes intermittent delays in data
transmission.
you could probably combine this with the other filter

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:315: It does this by inserting 3 20 second days when
transmitting
delays not days :) ... at least i hope

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:375: ] + port_flags)
.extend(port_flags)

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:394: ] + port_flags)
ditto

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh#newcode42
image_to_live.sh:42: "Have the client request from this porxy instead of
devserver."
porxies are delicious

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh#newcode221
image_to_live.sh:221: if [[ -n ${FLAGS_proxy_port} ]]; then
no need for double []

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode52
lib/cros_test_proxy.py:52: s_out.connect( (self.server.address_out,
self.server.port_out) )
additional spaces unnecessary

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode64
lib/cros_test_proxy.py:64: break
when would get either of these exceptions (here and below)?

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode78
lib/cros_test_proxy.py:78: def __init__(self,
i try to define this at the top of classes...up to you

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode103
lib/cros_test_proxy.py:103: 
two lines

[dgarrett, 2010-12-07 03:48:18]

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:137: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
On 2010/12/07 02:26:51, adlr wrote:
> 80 cols

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:274: def __init__(self):
On 2010/12/07 02:26:51, adlr wrote:
> please add a docstring for each method/function

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:380: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
On 2010/12/07 02:26:51, adlr wrote:
> 80 cols

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:460: port_flags = []
The changes I've added everywhere are identical, but the code I'm changing isn't
in any of the four cases. It felt like a common function would be as problematic
as leaving them separate.

On 2010/12/07 02:26:51, adlr wrote:
> there seems to be code duplication here and in the next few functions. can the
> common code be combined?

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:479: def UpdateUsingPayload(self, update_path,
stateful_change='old', proxy_port=None):
On 2010/12/07 02:26:51, adlr wrote:
> 80 cols

Done.

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode45
lib/cros_test_proxy.py:45: def setup(self):
On 2010/12/07 02:26:51, adlr wrote:
> please add a docstring for each method/function

Done.

[dgarrett, 2010-12-07 04:08:51]

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:133: exception of code needed for ssh.
On 2010/12/07 03:44:36, sosa wrote:
> doc for proxy_port

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:268: """This filter causes the proxy to interrupte
the download 3 times
On 2010/12/07 03:44:36, sosa wrote:
> spelling

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:296: proxy_port = 8081
On 2010/12/07 03:44:36, sosa wrote:
> Comment about the default port choices

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:313: """Causes intermittent delays in data
transmission.
On 2010/12/07 03:44:36, sosa wrote:
> you could probably combine this with the other filter

I could, but plan to make more changes that will cause them to diverge more.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:315: It does this by inserting 3 20 second days when
transmitting
On 2010/12/07 03:44:36, sosa wrote:
> delays not days :) ... at least i hope

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:375: ] + port_flags)
On 2010/12/07 03:44:36, sosa wrote:
> .extend(port_flags)

Done.

http://codereview.chromium.org/5632002/diff/1/bin/cros_au_test_harness.py#new...
bin/cros_au_test_harness.py:394: ] + port_flags)
On 2010/12/07 03:44:36, sosa wrote:
> ditto

Done.

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh#newcode42
image_to_live.sh:42: "Have the client request from this porxy instead of
devserver."
On 2010/12/07 03:44:36, sosa wrote:
> porxies are delicious

Done.

http://codereview.chromium.org/5632002/diff/1/image_to_live.sh#newcode221
image_to_live.sh:221: if [[ -n ${FLAGS_proxy_port} ]]; then
On 2010/12/07 03:44:36, sosa wrote:
> no need for double []

I had weird behavior in testing that was fixed by using the doubles, but I
couldn't figure out why. I agree that it shouldn't be needed.

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode52
lib/cros_test_proxy.py:52: s_out.connect( (self.server.address_out,
self.server.port_out) )
On 2010/12/07 03:44:36, sosa wrote:
> additional spaces unnecessary

Done.

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode64
lib/cros_test_proxy.py:64: break
On 2010/12/07 03:44:36, sosa wrote:
> when would get either of these exceptions (here and below)?

Usually because the connections are closed at the far end. I probably should
have figured out exactly what all exceptions could be thrown, but decided that I
really didn't care. Any error means close it all.

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode78
lib/cros_test_proxy.py:78: def __init__(self,
On 2010/12/07 03:44:36, sosa wrote:
> i try to define this at the top of classes...up to you

Done.

http://codereview.chromium.org/5632002/diff/1/lib/cros_test_proxy.py#newcode103
lib/cros_test_proxy.py:103: 
On 2010/12/07 03:44:36, sosa wrote:
> two lines

Done.

[sosa, 2010-12-07 17:49:22]

Looking nice.  Here are some more comments

http://codereview.chromium.org/5632002/diff/15001/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/15001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:334: # These values are reset for each connection
These two should be docstringed as well

http://codereview.chromium.org/5632002/diff/15001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:349: 
All the code below is common to both functions ... can you move into
commonProxyTest(cros_test_proxy.Filter)

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:1: #!/usr/bin/python
Is this meant to be called directly?  If it's just a library it shouldn't have
this at the top (reserved for things we call directly)

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:62: while True:
Is there any better way to end this?  i.e. create a close function that sets
some value in a critical section in another thread and watch for it here (in
another critical section)

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:70: # If there is any error sending data, close both
connections.
Is there a way to be more specific with the exception ... i.e. wait on a
specific type of error?

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:119: s = CrosTestProxy(port_in=2222, port_out=22,
filter=Filter())
Can you comment on what are these constants?

[dgarrett, 2010-12-09 00:48:38]

http://codereview.chromium.org/5632002/diff/15001/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/15001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:334: # These values are reset for each connection
On 2010/12/07 17:49:23, sosa wrote:
> These two should be docstringed as well

Done.

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py
File lib/cros_test_proxy.py (right):

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:1: #!/usr/bin/python
On 2010/12/07 17:49:23, sosa wrote:
> Is this meant to be called directly?  If it's just a library it shouldn't have
> this at the top (reserved for things we call directly)

Um... I had a main I used for initial testing, but should probably pull it out
now.

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:62: while True:
On 2010/12/07 17:49:23, sosa wrote:
> Is there any better way to end this?  i.e. create a close function that sets
> some value in a critical section in another thread and watch for it here (in
> another critical section)

The only reason I see for introducing another thread is so that incoming and
outgoing packets are handled separately. However, I find it kinda handy to have
them single threaded. It makes the delay type tests much easier to write, and
doesn't affect general performance enough to care.

http://codereview.chromium.org/5632002/diff/15001/lib/cros_test_proxy.py#newc...
lib/cros_test_proxy.py:70: # If there is any error sending data, close both
connections.
On 2010/12/07 17:49:23, sosa wrote:
> Is there a way to be more specific with the exception ... i.e. wait on a
> specific type of error?

Done.

http://codereview.chromium.org/5632002/diff/22001/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/22001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:381: ] + port_flags)
Turns out that extend returns None. I prefer keeping the expression inline, so +
works much better.

[sosa, 2010-12-09 23:51:01]

LGTM w/ nit

http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:127: 
Where'd this comment go?

# The devserver runs at port 8080 by default. We assume that here, and	
# start our proxy at 8081. We then tell our update tools to have the	
# client connect to 8081 instead of 8080.

[dgarrett, 2010-12-09 23:52:10]

Rebasing on top of your last change was ugly. I might have lost it in the
process.

On Thu, Dec 9, 2010 at 3:51 PM, <sosa@chromium.org> wrote:

> LGTM w/ nit
>
>
>
> http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py
>
> File bin/cros_au_test_harness.py (right):
>
>
>
http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py...
> bin/cros_au_test_harness.py:127:
> Where'd this comment go?
>
> # The devserver runs at port 8080 by default. We assume that here, and
> # start our proxy at 8081. We then tell our update tools to have the
> # client connect to 8081 instead of 8080.
>
>
> http://codereview.chromium.org/5632002/
>



-- 
Don

[sosa, 2010-12-09 23:55:42]

Sorrryyyy

On Thu, Dec 9, 2010 at 3:52 PM, Don Garrett <dgarrett@chromium.org> wrote:
> Rebasing on top of your last change was ugly. I might have lost it in the
> process.
>
> On Thu, Dec 9, 2010 at 3:51 PM, <sosa@chromium.org> wrote:
>>
>> LGTM w/ nit
>>
>>
>>
>> http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py
>> File bin/cros_au_test_harness.py (right):
>>
>>
>>
http://codereview.chromium.org/5632002/diff/30001/bin/cros_au_test_harness.py...
>> bin/cros_au_test_harness.py:127:
>> Where'd this comment go?
>>
>> # The devserver runs at port 8080 by default. We assume that here, and
>> # start our proxy at 8081. We then tell our update tools to have the
>> # client connect to 8081 instead of 8080.
>>
>> http://codereview.chromium.org/5632002/
>
>
>
> --
> Don
>