Please allow us to mock the Client used by _withClient

[Hixie]

We would like to be able to inject a client into the http package (e.g. the MockClient obtained from http/testing) such that the top-level methods such as "get" and "head" and so forth will use the mock client instead of a brand new Client.

We had previously implemented this by forking http and changing it as follows:
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/http/client.dart#L32

We would like to just use the http package directly now, but without this way to inject a MockClient, we have the problem that anyone using the convenience methods like http.get() will end up doing actual network traffic in our tests instead of hitting our mock.

[nex3]

This is why the Client class implements the same APIs as the top-level library methods: to make it easy to switch code using top-level APIs to take a Client as a parameter instead. The top-level APIs are there for scripting convenience, but any more advanced case like reusable libraries or applications that need to configure their requests should just use the Client API directly.

If you really want to use global state, you can write var http = new Client() at the top level and all your existing calls to http.whatever() should work, but I don't recommend that.

[Hixie]

The problem is we're trying to mock things out for libraries that we didn't write and don't yet know exist that use the top-level API.

Let me phrase the question a different way:

How are we supposed to test code that uses the top-level methods from the http package?

[nex3]

If you want to unit-test code like that, it should probably be re-written. Again, those methods are intended for quick-and-dirty scripts, and designed to be easy to swap out for explicit client calls for more advanced or robust use-cases.

[Hixie]

It would be really easy to make these APIs testable. What harm is there in allowing them to be tested?

[nex3]

It adds API surface and makes it harder to reason locally about code, and it encourages users to continue using the quick-and-dirty global APIs rather than switching to the more-explicit, more-efficient Client API.

[Hixie]

Could we have a way to disable the inefficient API?

Currently, the http package appears to provide a highly convenient, efficient-looking API. It is so attractive that people use it -- it's the only API we use in the flutter framework, for instance. If that API is in fact not the API we want people to use, we should either remove it, or provide a way to verify that it isn't being used (e.g. deprecate it, or have a flag we can set that makes it throw, or whatever).

[nex3]

It sounds like you want to ensure that all code you call goes through a particular HTTP API. I understand why that would be useful, but it's kind of at odds with the broader philosophy of encapsulation. Dart's language design and existing APIs generally operate on the philosophy that a chunk of code is customizable only in the ways it explicitly makes itself customizable. The move towards sealed classes and members in Dart 2 reinforces this further.

This philosophy can definitely be inconvenient when you want to modify code in ways that its authors don't expect, like overriding its HTTP client, but you get a lot in exchange. The API surface of a package is vastly smaller when monkey-patching isn't allowed, which makes it easier to avoid breaking changes and ends up making the whole ecosystem much more stable.

As an example, a package might switch from the global http helpers to a completely different implementation, like raw HttpClient calls. The author would reasonably expect this to be a non-breaking change, since they're only changing implementation calls. But if those http calls could be overridden globally, then that could very easily be a breaking change.

The good news is, if and when the author decides they want to move to a more customizable API, the http package makes that really easy by exactly duplicating the top-level API on the Client object. Change any reference to http from a namespace to a variable or field and you're good to go.

[Hixie]

What I want to ensure is that if someone decides to use Flutter and Dart official packages, and then does the obvious thing with them, that they end up with efficient and testable code. Currently, for Flutter, with our fork of the http package, this is possible (modulo the efficiency aspect, apparently), because the easiest APIs for doing HTTP traffic are testable.

We want to stop forking the http package. Unfortunately, doing so today means a regression in our usability, because as it stands today, using Flutter and this official package and then doing the obvious thing with it means you end up with untestable code.

We could solve this in several ways:

• Remove the untestable APIs.
• Allow the untestable APIs to be disabled.
• Mechanically deprecate the untestable APIs, so that the analyzer catches usage of those APIs.
• Make the untestable APIs less attractive.
• Add a way to test the API, as we used to have. This is about 5 lines of code to the http package, but does mean that it's harder to reason about the code.

I don't really mind how we solve the problem.

Currently there is a very attractive API in the http package, and yet this API is not testable (and apparently is less efficient). I do not believe making it testable is what would make it attractive. It is already attractive.

[nex3]

I think I may be operating on a broader understanding of "testable". Adding tests for a chunk of code always involves some amount of work—the tests need to be written, and when they uncover bugs those bugs need to be fixed. When using http, a (small) part of that work may be switching to the mockable version of the API. But the original code was still "testable" in the sense that adding tests was low-friction.

That said, it definitely seems that we should be clearer in our messaging about the downsides of the global functions, and the migration path to an explicit Client.

[Hixie]

By "testable" I mean "if the code was written without bugs, according to the documentation, in the most obvious and convenient way, then adding tests will not require any changes to the code".

[nex3]

From a concrete user-experience perspective, though, the difference between making a small modification to the base code and making no modification is very small relative to the effort of writing tests in the first place—especially since most untested code does in practice have bugs, so the base code will probably need to change anyway.

[Hixie]

The difference is that if the API isn't testable, then I don't want anyone on the team using it, even by mistake, even if they don't write tests for it, because we know for a fact that the code cannot be correct -- it will definitely, at some point, have to change (when we test it).

So either the analyzer should report it (@deprecated), or it should be something we can disable (and thus cause to fail, so they won't use it), or we should remove it (so it won't even show up in the docs), or we should make it testable. I really don't mind which.

[matanlurey]

Maybe I misunderstand something, but I'm on @nex3's side on this one.

Imagine asking to mock new DateTime(). Clearly that isn't possible - we do have a new different package, either quiver or clock that gives you a wrapper object that can be mocked. Similarly we have package:file (mostly thanks to Todd, but I started it) which gives you a wrapper API that can be mocked. This is pretty consistent with most static languages (Java, C#, Swift, etc).

At least for UI frameworks, I find dependency injection useful here:

class WidgetThatNeedsHttp {
  final Client _http;

  WidgetThatNeedsHttp(this._http);
}

Instead of hoping that global functions are mockable.

[Hixie]

package:file is ample evidence that dart:io is poorly designed. Having to wrap an entire package because otherwise code using that package is untestable is a clear indication that that package has fundamentally failed. We literally can't (and don't) directly use dart:io because of this. For the flutter tool we literally have a test that greps our codebase looking for stray imports of 'dart:io' and fails our build if someone accidentally adds one. We have to wrap everything we do with an expensive thunk layer. I have no interest in doing this for the http package. The whole point is we're trying to move away from forking it.

I'm not sure what it would mean to mock DateTime as a whole. Certainly the fact that you can't test code that uses DateTime.now directly is a major failing of that API. We have code that uses DateTime.now, and it's untested (and probably buggy) because that API is untestable. It's among the remaining 20% of the flutter framework codebase to not have coverage.

The existence of other poorly designed APIs in the Dart ecosystem is not a good justification for following that pattern here, IMHO, especially given how easy it would be to fix.