Typing and testing

Help & Support
1

I’ve been pondering this lately, also in relation to the talk about more explicit types:

Let’s say we’re making a small tool that needs to talk to GitHubs API. And let’s pretend someone has made an awesome Github shard. It has everything, pass it and token and you can request repos, issues, deployment, the works, and it’ll hide all the nitty gritty HTTP requests for you. Nicely encapsulated, just how we like it.

So we’re making this small tool that, let’s say, asks Github who you are and what repos you have in order to clone them all. So it just needs information from two endpoints in the API (something like user/me and <org>/repos). Which translates to just to method calls on the Github client instance provided by the previously mentioned shard.

Obviously we’d like to test your own code, so we inject the Github client into our own worker class so we can test our class without relying on the shard and in the end a working internet connection to Github.

But… Even when injecting the class, we’re typing it to Github::Client, as the compiler will insist on this when we put the client in an instance variable. So we’re back to square one.

How does people generally deal with this in practice? Make a module with the methods we really need and add it to the Github::Client class, so we can have our own mock that just needs to implement that interface? Or are there better tricks I haven’t discovered yet?

2

I think this is the best option at the moment. You have a single interface your code depends on with two implementations of it. One being essentially a wrapper of the official client and another used for testing. The former of which could either be directly monkey patched in, or by decorating the official one and merely delegating the method calls to it. I’m personally a fan of the latter just so you avoid touching code you don’t own at all.

I also think this ties in nicely with how things could be with the idea around [RFC] Standardized Abstractions. Namely the idea that the Github::Client accepts an optional argument of the HTTP::Client that is actually used to make the requests. Under normal usages this would be HTTP::Client, but for purposes of your test, it could be HTTP::MockClient which works something like Testing Guzzle Clients — Guzzle Documentation, allowing you to customize the responses and access the requests made by the underlying client. It could still make sense for your code to depend on some internal interface, but that’s for another discussion.

It’s also entirely possible this could be done without the need for a dedicated interface by virtue of Redesign HTTP::Client to implement missing features · Issue #6011 · crystal-lang/crystal · GitHub of which the Mock connections (for testing) could possibly be a subclass of the built-in HTTP::Client.

IMO this is just a perfect example of how the ideas around dependency injection can be so useful for stuff like this.

2 Likes
3

Decorators certainly have its uses when dealing with clients that’s basically just a thin wrapper around the API and the implementor haven’t had any thought about how to actually use the client. Or require you to make work your way through multiple calls to get at the information you’re after. Then you can have your decorator cut it down to an API closer to what your app needs.

Well, if you’re just including an all-abstract-defs module, you’re barely touching it.

It does make one think fondly of the Go way of dealing with interfaces, where a class doesn’t have to explicitly state it implements an interface, but just provide the same signature.

One very interesting thing about Gos interfaces, apart from the implicitity thing, is that they’re reversed compared to pretty much any language I can think of (which might not be much). It’s the calling module that defines the interface, not the called module.

This flipping about means that interfaces is more about what someone needs from the called module rather than what the called module provides. Which means, I’m told, that Go has less, but more focused interfaces. And each exists because someone is using it.

But I digress. Magic interfaces or not, it becomes unwieldy when you try to apply the same principles to your own classes in order to keep them testable. Whenever you add a public method to a class that’s used by some other class (which should cover a good chunk of any given codebase), you have to add it to the interface. Together with all the overloads. Which rather goes against the DRY principle.

Ah, but I’d rather not be testing whether Github::Client parses responses properly, as that’s someone else’s job. I could just use webmock, but then I’d have to produce proper Github responses for each test and deal with changes in the API (which the Github shard maintainer, hopefully, quietly hides from me). Sure, I’d want an integration test or two that tests the full stack, but I don’t want to fake Githubs voluminous JSON responses in each unit test. So a pluggable HTTP::Client doesn’t really help me out here.

I threw out the interface idea as, coming from PHP, DI is the hammer I reach for, but it’s not really something I see in Crystal code, so I wondered how Crystalists deal with this.

4

But you still are touching it. The main issue this brings is it tightly couples your interface to their interface. Say for example the shard adds a new optional parameter, or renames a parameter, or changes the name of a method, all of which are reasonable things that could happen. When/if this happens, you now have to go and adjust your interface to match the shard’s. Which could require you to go through all your usages of that interface to apply those changes.

On the other hand, if you used a decorator type to implement your interface, you’d still have to possibly change how you’re calling into the shard, but only in that one type since nothing about your interface changed so everything depending on it remains the same.

When to add an interface to a class — Matthias Noback - Blog this blog post made a lot of sense to me for this kind of stuff. Is a good read.

The blog post actually calls this out as well. You’d only need an interface if you don’t want to expose every public method as there would be no benefit in having an interface that just mirrors the public methods of that class. At that point the type of the class is the interface. This ofc doesn’t include other contexts where you may need it, such as wanting multiple implementations for a specific thing or what have you.

That’s definitely fair, and would be a con of the second approach, but also points back to my It could still make sense for your code to depend on some internal interface, but that’s for another discussion comment. Doing both gives you the most flexibility in determining what makes the most sense for what and how you want to test/do.

My 2 cents on this is it IS the solution/correct answer, but you don’t see it in Crystal code because a majority of Crystal developers are probably from Ruby backgrounds which don’t use the pattern as much as those in PHP/other languages.

1 Like
5

I’ve duck-typed wrappers like this:

class TestGitHubClient
end

class GitHub::Client
end

class MyGitHubClient(T)
  @github : T

  def self.test_client
    new TestGitHubClient.new
  end

  def self.new
    new GitHub::Client.new
  end

  def initialize(@github)
  end

  forward_missing_to @github
end

pp test: MyGitHubClient.test_client, live: MyGitHubClient.new

Your test config would instantiate with .test_client, but the config for the live app would call .new. Your test client would only need to implement the methods you’re using in the app. And you’ll know if you mess up the interface of the test client because either the specs or the real app won’t compile.

2 Likes
6

As I would if I was just using the shard directly. The point of the exercise wasn’t to protect myself against changes to the shard, but just to make my own class testable.

And now we have two interfaces, the part of the shard we’re using and our own, almost the same, but slightly different.

Good read, but what’s a given for PHP might not hold for Crystal. He lost me at “For everything else: stick to a final class”, I have… Issues… With that.

I agree with that, but the point here was that we needed the interface in order to being able to inject something in a test that is not the class. But I’d be happier with something like MyMock.masquerade_as Github::Client (but that would require cooperation from the compiler).

Well, my take is that any abstraction comes with a cost, and art is in selecting when, and when not, to use it. The FizzBuzzEnterpriseEdition is the tongue in cheek example of what happens if you strictly adhere to dogma.

@jgaskins
Oooh, very excellent hack there.

7

Right, which doing what you originally described would do just that by making your code depend upon an abstraction not a concrete implementation of it. The other ideas/suggestions around using a decorator type have added benefits that I personally like but are not necessarily required to solve the problem described in the OP.

Kinda, not really? The one from the shard you’re using is implicit while the one you define is explicit. But there’s no real problem with having them be the same. The important part is your code just not depending upon a specific implementation such that you can use a mock client for testing and still be compatible.

Right, of which this is a perfect use case for using it. I’m not suggesting that Crystal needs to be Java by having interfaces for absolutely everything. But I also don’t think the solution is to be like “ahh interfaces bad” and instead propose adding more complexity to the compiler in the form of masquerade_as or what have you. Especially when the alternative is as simple as:

module MyApp::GithubClientInterface; end

class MyApp::Spec::MockGithubClient
  include MyApp::GithubClientInterface
 
  # ...
end

class MyApp::GithubClient
  include MyApp::GithubClientInterface

  # ...
end

class MyApp::Worker
  def initialize(@github_client : MyApp::GithubClientInterface); end
  
  # ...
end

Which doesn’t require any changes to the language, and is more straightforward/common pattern already. Ultimately there is no wrong solution. While it seems we just have differing views on how to design software. I personally like taking the extra steps to make it so if i ever need to switch github clients, i only need to update 1 thing and not everything. It’s ultimately up to you if you think that’s also worth it. Otherwise re-open that shard’s client, throw in the interface include, and call it a day :person_shrugging:.

I mean hell, you don’t even need to define any abstract methods (tho I’d still consider it a best practice), as @jgaskins’s example points out the compiler will just know and error if one of the implementers doesn’t implement one of the methods.

1 Like
8

But the implicit interface you’re talking about is the shards explicit interface. You might get away with only dealing with your own most of the time, but you’ll have two very explicit ones when you’re working on your decorator. And if you’re fixing upstream changes in the decorator, they’re slowly drifting apart and you end up having to look up methodX each time as you can’t remember which one had the arguments in which order.

Oh, right… I’m so indoctrinated that specifying an interface on a parameter and then calling some method not part of that interface is unthinkable. I’m getting the chills just thinking about it. Which is funny as I’m raised on PHP which… Doesn’t care.

Talked with my colleague which has a thing for Go, and according to him Go does check if you’re using the parameter according to the type you claimed, and that’s why it can compile a module and just link it in.

Well, I was thinking of it like some preprocessor that would just make the compiler replace any type of Github::Client with Github::Client | MyMock. But granted, if we start messing with the compiler, Go-like interfaces is more interesting, if they’re “type enough” for incremental compilation to become feasible.

Wouldn’t go that far, but just because they’ve become the answer to some problems in languages where’s there’s really no other choice, I don’t think that Crystal should just accept and adopt that dogma without considering if there might be better alternatives. Crystal doesn’t have 20 years of legacy production code it needs to keep running, so it can afford to try out some things.

9

In the hope that someone else than me might learn something from this thread, here’s a summary of what I’ve learned trying to apply the above thoughts in practice, in my pet project that talks to, not one, but two, external services.

Of course @Blacksmoke16 is right that decorators is generally a good thing. Not so much for testing, but for getting the nitty-gritty of an interface to better match ones needs.

To take the hypothetical app from above, the tool itself really don’t care about Github, users or repos, it just want to get a list of clonable URLs. So if we add in a Forge module with just a getUrls(user), we’ll cut down on the housekeeping in the tool code and focus on the task at hand. That’s what programming is about, to abstract complicated things down to simpler things.

The attentive reader will have noticed that we’re just one step away from being able to switch the GithubForge class (which includes the module) with another that talks to, say, Gitlab instead, without having to change much, if anything, in the tool itself (we’ll skip over the detail of what a user is for the time being), but that’s just an added bonus.

But in regard to testing, we’ve just moved the problem. We can test the tool by handing over a mock forge (by depending on the module rather than a concrete class), but the GithubForge class now contains a significant part of our business logic (in regard to the Github client), which we’d rather like to test.

But if GithubForge has a direct coupling with Github::Client, we’re back to having to use webmock to test it. Apart from having to construct mock responses that satisfies Github::Client, some cases might simply not be mockable (for instance I don’t think webmock can simulate a “Host not found” error).

But @jgaskins hack to the rescue. By making GithubForge a generic class that takes the client as a type parameter, we can have our cake and eat it too:

class GithubForge(Client)
  def self.for(user : User, klass : Client.type = Github::client)
    new(user, klass.new)
  end

  def initialize(@user : User, @client : Client)
  end
end

Easy to inject a mock, but it defaults to the real client. Works marvelously.

1 Like
10

Not read the comment carefully, but i guess following links maybe help for this discuss.