Add a mock HTTP client class.

Add a mock HTTP client class.

Committed: https://code.google.com/p/dart/source/detail?r=14546

[Bob Nystrom, 2012-11-05 20:36:04]

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart#newcode69
pkg/http/lib/http.dart:69: export 'src/mock_client.dart';
I like this class a lot, but I think it shouldn't be exposed by the main http
library. How about 'package:http/testing.dart' ?

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
File pkg/http/lib/src/mock_client.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
pkg/http/lib/src/mock_client.dart:26: set streamHandler(MockClientStreamHandler
fn) => _handler = fn;
Setters that don't have getters are a design smell for me. I think users expect
to be able to access "properties" that they can modify.

A couple of options:

1. Add getters. That's easy for streamHandler, but a little trickier for
handler.
2. If these aren't properties you actually expect users to mutate, make them
constructor parameters instead.
3. Make them methods.

Thoughts?

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
pkg/http/lib/src/mock_client.dart:63: var redirectCount = 0;
Remove this?

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
File pkg/http/test/mock_client_test.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
pkg/http/test/mock_client_test.dart:11: import '../../unittest/unittest.dart';
Add a TODO here that this should be a 'package:' import.

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
pkg/http/test/mock_client_test.dart:55: expect(future,
completion(equals('Request body was "hello, world"')));
You've organized this differently from the expect() call in the first test
(which just does the future inline). Any particular reason?

[nweiz, 2012-11-05 22:35:15]

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart#newcode69
pkg/http/lib/http.dart:69: export 'src/mock_client.dart';
On 2012/11/05 20:36:04, Bob Nystrom wrote:
> I like this class a lot, but I think it shouldn't be exposed by the main http
> library. How about 'package:http/testing.dart' ?

Done.

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
File pkg/http/lib/src/mock_client.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
pkg/http/lib/src/mock_client.dart:26: set streamHandler(MockClientStreamHandler
fn) => _handler = fn;
On 2012/11/05 20:36:04, Bob Nystrom wrote:
> Setters that don't have getters are a design smell for me. I think users
expect
> to be able to access "properties" that they can modify.
> 
> A couple of options:
> 
> 1. Add getters. That's easy for streamHandler, but a little trickier for
> handler.
> 2. If these aren't properties you actually expect users to mutate, make them
> constructor parameters instead.
> 3. Make them methods.
> 
> Thoughts?

Made them constructor parameters.

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/src/mock_client....
pkg/http/lib/src/mock_client.dart:63: var redirectCount = 0;
On 2012/11/05 20:36:04, Bob Nystrom wrote:
> Remove this?

Done.

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
File pkg/http/test/mock_client_test.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
pkg/http/test/mock_client_test.dart:11: import '../../unittest/unittest.dart';
On 2012/11/05 20:36:04, Bob Nystrom wrote:
> Add a TODO here that this should be a 'package:' import.

Done.

https://codereview.chromium.org/11362097/diff/1/pkg/http/test/mock_client_tes...
pkg/http/test/mock_client_test.dart:55: expect(future,
completion(equals('Request body was "hello, world"')));
On 2012/11/05 20:36:04, Bob Nystrom wrote:
> You've organized this differently from the expect() call in the first test
> (which just does the future inline). Any particular reason?

Just aesthetics. Since the future includes a couple one-line transformations, it
looked better out-of-line.

[Bob Nystrom, 2012-11-05 23:01:31]

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11362097/diff/1/pkg/http/lib/http.dart#newcode69
pkg/http/lib/http.dart:69: export 'src/mock_client.dart';
On 2012/11/05 22:35:16, nweiz wrote:
> On 2012/11/05 20:36:04, Bob Nystrom wrote:
> > I like this class a lot, but I think it shouldn't be exposed by the main
http
> > library. How about 'package:http/testing.dart' ?
> 
> Done.

Did you upload this change? I don't see it.

[Bob Nystrom, 2012-11-05 23:25:15]

One question. Either way, LGTM!

https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart
File pkg/http/lib/testing.dart (right):

https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart#...
pkg/http/lib/testing.dart:27: export 'src/mock_client.dart';
Is there any reason you're leaving the source there instead of just moving it
here?

[nweiz, 2012-11-05 23:40:15]

https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart
File pkg/http/lib/testing.dart (right):

https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart#...
pkg/http/lib/testing.dart:27: export 'src/mock_client.dart';
On 2012/11/05 23:25:15, Bob Nystrom wrote:
> Is there any reason you're leaving the source there instead of just moving it
> here?

It indicates that this library isn't intended to just be the MockClient. In the
future I'll probably add other testing utilities, and they'll go here as well.

[Bob Nystrom, 2012-11-05 23:44:55]

On 2012/11/05 23:40:15, nweiz wrote:
> https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart
> File pkg/http/lib/testing.dart (right):
> 
>
https://codereview.chromium.org/11362097/diff/7001/pkg/http/lib/testing.dart#...
> pkg/http/lib/testing.dart:27: export 'src/mock_client.dart';
> On 2012/11/05 23:25:15, Bob Nystrom wrote:
> > Is there any reason you're leaving the source there instead of just moving
it
> > here?
> 
> It indicates that this library isn't intended to just be the MockClient. In
the
> future I'll probably add other testing utilities, and they'll go here as well.

Perfect then. I was thinking something similar, hence suggesting testing.dart
instead of mock.dart or something like that.