Basic network abstraction implementation...

[criske]

Here is a basic abstraction of a http client, that could be used in the future: https://gist.github.com/criske/bb02e7f2fc5f862dae0f30396b6fc9dd And here is the implementation at work. GithubIssues fully unit tested: https://gist.github.com/criske/79fa44cc2379da1e47153c1293a92a2e

Edit: here is GithubIssues injected with http client interface: https://gist.github.com/criske/67657937f324a987646ac7a45edd0318

[amihaiemil]

@criske As far as I can see, this abstraction only helps us to write unit tests for the Github API calls.

But we can already write those tests by mocking an HTTP Server, starting it on a random port and instantiating Github with the URI http://localhost:{mockServerPort}/....

We can do that quite elegantly, I've already done it in a few other projects and it's certainly less code than mocking that Http abstraction...

[amihaiemil]

@criske Here is an example with a Grizzly mock server: https://github.com/jcabi/jcabi-github/blob/89ad215653c76796a955734d05a4632f7794c82a/src/test/java/com/jcabi/github/RtGistTest.java#L70

1 start the server 2 specify mock answers/responses (so you can test all sorts of responses coming from "Github"). 3 you can also take the Request that has reached the mock server and make assertions on it, check if you have built it correctly.

[amihaiemil]

@criske But at the moment, unit tests for the network api calls are not urgent, we already have integration tests which are enough for now. I'd rather focus on getting the first version up and running.

[criske]

@criske As far as I can see, this abstraction only helps us to write unit tests for the Github API calls.

@amihaiemil Doesn't matter, can handle any calls...

At my last workplace we're heavly relying on https://github.com/square/okhttp/tree/master/mockwebserver to mock a server. Anyway, in the mobile world for example, writting native network code in the domain layer of a project based on clean-like architecture is a big no-no. We're always asked to hide that code behind an interface because: -if that code has android specific code, the logic can't be tested unless you run it an emulator or a real device. So to test that logic on the local machine, we're just using the implementation based on the local JVM; and only after that we were free to use a mock server (like Grizzly or any...)
-most of android devices are using Java 7 so you have to deal with HttpURLConnection; so, if at one point we changed to a httplib like OkHttp, we didn't need to refactor the whole code-base, only the Dagger injection modules. Ofc nowadays most of apps are using Retrofit, which is based on Okhttp, but that's another story.... But I get your point, is much simpler outside the android ecosystem. :D

[amihaiemil]

@criske

Doesn't matter, can handle any calls...

I know it can handle any calls, but it provides very little added value as opposed to what we have now. That abstraction is basically just a layer on top of JDK11's HttpClient.

If you want to make some abstractions, they should be made so that objects Github and Gitlab see absolutely nothing about any HTTP call being made -- but again, we don't need that now.

[amihaiemil]

@criske

About okhttp and other HttpClient libs: the main reason we're using Java 11 is that it has a built-in HttpClient in the JDK, so we don't need 3rd party libraries for that :)

[criske]

If you want to make some abstractions, they should be made so that objects Github and Gitlab see absolutely nothing about any HTTP call being made -- but again, we don't need that now.

Exactly, using a gateway interface or a repository (actually Storage is following this pattern, if I'm correct).

[amihaiemil]

Yes, Storage is designed to be the highest abstraction when it comes to storing our objects. self-core doesn't care whether it's in-memory, sql, no-sql, whatever :D

[amihaiemil]

fixed in #226