Spit and polish on fake roundtrippers.

Spit and polish on fake roundtrippers.

We have a facility to inject fake HTTP interactions in tests, but it's harder
to figure out and use than I think it needs to be.  This branch:

 * Renames VirtualRoundTripper to CannedRoundTripper, to reflect more
   specifically that it returns canned responses.  Still not great.

 * Replaces the slice-of-structs-of-name-and-value way in which the
   roundtripper stores canned responses with a simpler
   map-of-names-to-contents.
 
 * Hides the details of how a roundtripper is installed inside a helper
   function.
 
 * Documents how all this works, and in particular, a few nasty gotchas w.r.t.
   how conflicting matches are handled.

 * Extracts a factory for HTTP responses.  I think it brings out the logic in
   the caller more clearly, eliminating some repetition along the way.

 * Gives me personally the chance to learn about a mechanism I'll be using
   next.

https://code.launchpad.net/~jtv/juju-core/encapsulate-proxyroundtripper/+merge/177338

(do not edit description out of merge proposal)

[jtv.canonical, 2013-07-29 09:15:29]

Please take a look.

[dimitern, 2013-07-29 10:02:35]

Very nice! LGTM with a bunch of trivial suggestions below

https://codereview.appspot.com/12018043/diff/1/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/12018043/diff/1/cmd/jujud/bootstrap_test.go#ne...
cmd/jujud/bootstrap_test.go:53: testRoundTripper.Sub =
jujutest.NewCannedRoundTripper(map[string]string{
Can you please extract the map[string]string... part in a variable before this
line? So:

args := map[string]string{"/" + environs.StateFile: string(stateData)}
testRoundTripper.Sub = jujutest.NewCannedRoundTripper(args, nil)

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go
File environs/jujutest/metadata.go (right):

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:16: // Use this in conjunction with
ProxyRoundTripper.  A ProxyRoundTripper is

s/.  A/. A/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:19: // CannedRoundTripper.  The reason for this is
that we can register a

s/.  The/. The/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:24: // files maps file names to their contents. 
If the roundtripper
s/.  If/. If/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:28: // TODO: Do something more sensible here:
either make files take

TODO(jtv): ...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:34: // codes.  If a request's URL matches any of
these prefixes, the

s/.  If/. If/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:36: // There is no clever longest-prefix selection
here.  If more than

ditto

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:38: // TODO: Decide what to do about multiple
matching prefixes.

TODO(jtv):...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:45: // delegate to another RoundTripper.  This
lets tests change how they handle

s/.  This/. This/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:51: // for a particular protocol.  For example, if
you call

/.  For/. For/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:65: // This cannot be undone, nor overwritten with
a different roundtripper.  If

How about adding a Disable() or Unregister() method that internally sets Sub to
nil? This way you don't need to explain all that here, just point to the method
and it'll work as expected, right?

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:96: // Return a canned error or body for the given
request.

// RoundTrip returns...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:111: func NewCannedRoundTripper(files
map[string]string, errorURLs map[string]int) *CannedRoundTripper {

// NewCannedRoundTripper creates a new CannedRoundTripper. ?

[dimitern, 2013-07-29 10:02:35]

Very nice! LGTM with a bunch of trivial suggestions below

https://codereview.appspot.com/12018043/diff/1/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/12018043/diff/1/cmd/jujud/bootstrap_test.go#ne...
cmd/jujud/bootstrap_test.go:53: testRoundTripper.Sub =
jujutest.NewCannedRoundTripper(map[string]string{
Can you please extract the map[string]string... part in a variable before this
line? So:

args := map[string]string{"/" + environs.StateFile: string(stateData)}
testRoundTripper.Sub = jujutest.NewCannedRoundTripper(args, nil)

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go
File environs/jujutest/metadata.go (right):

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:16: // Use this in conjunction with
ProxyRoundTripper.  A ProxyRoundTripper is

s/.  A/. A/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:19: // CannedRoundTripper.  The reason for this is
that we can register a

s/.  The/. The/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:24: // files maps file names to their contents. 
If the roundtripper
s/.  If/. If/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:28: // TODO: Do something more sensible here:
either make files take

TODO(jtv): ...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:34: // codes.  If a request's URL matches any of
these prefixes, the

s/.  If/. If/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:36: // There is no clever longest-prefix selection
here.  If more than

ditto

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:38: // TODO: Decide what to do about multiple
matching prefixes.

TODO(jtv):...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:45: // delegate to another RoundTripper.  This
lets tests change how they handle

s/.  This/. This/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:51: // for a particular protocol.  For example, if
you call

/.  For/. For/

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:65: // This cannot be undone, nor overwritten with
a different roundtripper.  If

How about adding a Disable() or Unregister() method that internally sets Sub to
nil? This way you don't need to explain all that here, just point to the method
and it'll work as expected, right?

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:96: // Return a canned error or body for the given
request.

// RoundTrip returns...

https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
environs/jujutest/metadata.go:111: func NewCannedRoundTripper(files
map[string]string, errorURLs map[string]int) *CannedRoundTripper {

// NewCannedRoundTripper creates a new CannedRoundTripper. ?

[rvb, 2013-07-29 10:14:02]

LGTM

[0]

220	+// ProxyRoundTripper is a RoundTripper

Might be worth replacing RoundTripper with http.RoundTripper to make it clear
this is the interface from the std library you're talking about.

[1]

I second Dimiter's suggestion to introduce a Disable() or a Unregister() method.
 It will make things more simple to understand and use.

[jtv.canonical, 2013-07-29 10:33:18]

On 2013/07/29 10:02:35, dimitern wrote:

>
https://codereview.appspot.com/12018043/diff/1/cmd/jujud/bootstrap_test.go#ne...
> cmd/jujud/bootstrap_test.go:53: testRoundTripper.Sub =
> jujutest.NewCannedRoundTripper(map[string]string{
> Can you please extract the map[string]string... part in a variable before this
> line? So:

You're right, that's better.


>
https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
> environs/jujutest/metadata.go:16: // Use this in conjunction with
> ProxyRoundTripper.  A ProxyRoundTripper is
> 
> s/.  A/. A/

Personally I find the double-spaced style easier on the eyes, but if there's a
rule against them I'll try to stop myself.

(Don't believe that stupid rant on the internet that everybody's always quoting
as gospel — it basically starts with the assumption that monospace fonts don't
exist and goes downhill from there).


>
https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
> environs/jujutest/metadata.go:28: // TODO: Do something more sensible here:
> either make files take
> 
> TODO(jtv): ...

Done.


>
https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
> environs/jujutest/metadata.go:65: // This cannot be undone, nor overwritten
with
> a different roundtripper.  If
> 
> How about adding a Disable() or Unregister() method that internally sets Sub
to
> nil? This way you don't need to explain all that here, just point to the
method
> and it'll work as expected, right?

I like the idea, but I have a feeling that there's much more detail to hide
here.  It'd take a fresh round of thinking to get that right.  I'm not even sure
this whole module needs to expose anything more than a single function for "here
are some canned responses I expect to see in my tests; now give me a protocol
that I can direct their requests to."  So I'm leaving something for the next
person who gets the urge to clean up.  :-)

(Tragically, Unregister() would not be a usable name because it'd give off the
false impression that it returns you to a state where it's OK to register again.
 TBH I don't even know how these different tests can all register roundtrippers
for the same scheme!)


>
https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
> environs/jujutest/metadata.go:96: // Return a canned error or body for the
given
> request.
> 
> // RoundTrip returns...

Fixed.

 
>
https://codereview.appspot.com/12018043/diff/1/environs/jujutest/metadata.go#...
> environs/jujutest/metadata.go:111: func NewCannedRoundTripper(files
> map[string]string, errorURLs map[string]int) *CannedRoundTripper {
> 
> // NewCannedRoundTripper creates a new CannedRoundTripper. ?

Done.  I had sort of picked up the impression that it wasn't idiomatic to
document constructors in Go, but I do like the habit of consistent
documentation.

[jtv.canonical, 2013-07-29 10:36:03]

Please take a look.

[jtv.canonical, 2013-07-29 10:40:57]

On 2013/07/29 10:14:02, rvb wrote:

> 220	+// ProxyRoundTripper is a RoundTripper
> 
> Might be worth replacing RoundTripper with http.RoundTripper to make it clear
> this is the interface from the std library you're talking about.

Thanks.  Done.


> I second Dimiter's suggestion to introduce a Disable() or a Unregister()
method.
>  It will make things more simple to understand and use.

Leaving that additional improvement for a future gardener, since I have a
feeling that a much more radical simplification could be in the cards.