Make myradio-go more mockable #22
Conversation
This allows mock requests to be created.
This is a pretty useless test, but at least it can now be run.
There was a problem hiding this comment.
It's rather... verbose for what's essentially just checking that json is being marshalled correctly. There's not even any guarantee that it follows what MyRadio outputs either, unless someone keeps that up to date...
Of course, I have no idea how you might improve on that...
| } | ||
|
|
||
| if !reflect.DeepEqual(showMeta, expected) { | ||
| t.Errorf("expected:\n%v\n\ngot:\n%v", expected, showMeta) |
There was a problem hiding this comment.
Seems like this could get a little difficult to decipher in the case that they differ. Perhaps make it a more specific error? Can't say I'm sure how you'd do that though..
There was a problem hiding this comment.
Without actually enumerating through all of the fields of showMeta, not sure how. This is verbose as it is :/
|
|
||
| // NewSession constructs a new Session with the given API key. | ||
| func NewSession(apikey string) (*Session, error) { | ||
| url, err := url.Parse(`https://ury.york.ac.uk/api/v2`) |
There was a problem hiding this comment.
wait, why was this ury.york.ac.uk in the original? Also, why isn't configurable?
There was a problem hiding this comment.
I feel as if making this configurable would be an entirely new prq :p Also I have no idea.
| baseurl: *url, | ||
| }, nil | ||
| // AuthedRequester answers API requests by making an authed API call. | ||
| type authedRequester struct { |
There was a problem hiding this comment.
Is it worth bothering to call it an "authedRequester" ? Why not just "Requester"?
There was a problem hiding this comment.
First, it's not meant to be public, so it shouldn't be capitalised :p
Second, idk, it seems weird to call a specific instance of a given interface (in this case apiRequester something more generic-sounding than its interface. Maybe the interface should be requester and this apiRequester or something.
| @@ -0,0 +1,119 @@ | |||
| package myradio | |||
There was a problem hiding this comment.
Hmm. I'm more of a fan of black box testing with go modules tbh, so this should be myradio_test - can you fiddle it so that's possible?
|
@LordAro short of having the myradio-go tests spin up an entire implementation of the MyRadio API, I'm not sure how these tests can do anything other than just checking the interpretation of JSON. (Some API methods do more than just unmarshal the JSON, the test here was just a quick proof of context). I'd argue that even making sure the JSON marshalling works can be useful. First, until I did the lint branch the track marshalling was completely broken and nobody noticed. Second, it ensures that if we change If anyone can find a way of making it less verbose, then sure. I picked a bad example in hindsight: show metadata is inherently very verbose! |
|
probably. |
This PRQ:
Sessioninto two things, aSessionobject that handles API requests from clients, and anapiRequesterinterface that actually does the API request;Sessionbehaviour intoauthedRequester(which implementsapiRequester);mockRequesterthat pretends to do an API request but instead returns a stockjson.RawMessage;Sessions to be created withmockRequestersusing the newMockSessionconstructor;Thus, any of myradio-go's existing methods can now be sent a JSON file directly instead of having to make an API call, which makes testing the myradio-go specific logic much easier!
This might seem a little overengineered, but it's the easiest way I could think of to make this possible.
The reason
Sessionstill exists, instead of usingapiRequesterdirectly, is backwards compatibility; allowing the myradio-go API to remain method-based; allowing helper methods likeapiRequestto be defined without changing the requesters; and allowing future expansion of what lives inSession.Things that would be nice:
show_test's JSON into a separate file;curlthough.