This time, I would like to introduce how to write code that is difficult to unit test. The language of the code is Java (please read other languages).
I also wrote an article that refactors what I wrote in this article. https://qiita.com/oda-kazuki/items/b66fe3d4efec822497e6
The degree of cohesion, degree of coupling, and cyclomatic complexity are also summarized below. https://qiita.com/oda-kazuki/items/a16b43dc624429de7db3
This time, the following processing is assumed.
In addition, in order to simplify the description, the external library of the following interface shall be used.
public class HttpClientFactory {
/**Returns a client for HTTP communication*/
static Client createClient(String domain);
}
public interface Client {
/**Get with Web API*/
HttpResult get(String path) throws HttpException;
/**Post with Web API*/
HttpResult post(String path, Json json) throws HttpException;
}
public class HttpResult {
/**Get the JSON Response Body*/
public Json body();
}
public class JsonMapper<T> {
/**Serialize the object to JSON*/
public Json serialize(T obj);
/**Deserialize JSON into an object*/
public T deserialize(Json json);
}
First, let's create a cache. Here is ** [Singleton] using the design pattern (https://ja.wikipedia.org/wiki/Singleton_%E3%83%91%E3%82%BF%E3%83%BC%E3%83%B3 ) Let's do it brilliantly with the pattern **. I'm cool to use design patterns.
Cache.java
public class Cache {
private static Cache instance = new Cahce();
//Commitment point 1:Effective use of Map
private Map<String,Object> user;
private String token;
private String accountId;
private Cache() {
}
public static Cache getInstance() {
return instance;
}
public void setUser(Map<String,Object> u) {
this.user = u;
}
public Map<String, Object> getUser() {
return this.user;
}
public void setToken(String a) {
this.token = a;
}
public String getToken() {
return this.token;
}
public String setAccountId(String accountId) {
this.accountId = accountId;
}
public String getAccountId() {
return this.accountId;
}
}
Next, write the process.
LoginService.java
public class LoginService {
//Commitment point 2:Initialize inside the class
private JsonMapper<Map<String,Object>> mapper = new JsonMapper<>();
//Commitment point 1:Use member variables in vain
private HttpException ex = null;
private Map<String,Object> user = null;
private Map<String,Object> body = null;
private String accessToken = null;
private Json json = null;
public Map<String,Object> login(String accessToken) {
//Commitment point 1:Reuse member variables
this.accessToken = accessToken;
this.account = null;
//Commitment point 1:Reuse local variables
Map<String,Object> user = null;
this.ex = null;
boolean result = true;
this.json = null;
this.body = null;
if(this.accessToken != null) {
getCache().setToken(accessToken);
} else {
this.accessToken = getCache().getToken();
}
//Commitment point 3:Full of nesting
for (int i = 0; i < 3; i++) {
if (getCache().getUser() != null) {
result = this.logout();
if (result) {
try {
//Commitment point 1:I don't know when the value was entered in the body just by looking here
this.getAccountId();
if (this.body != null) {
this.getAccount();
this.account = this.body;
if (this.body != null) {
getCache().setAccountId(this.body.get("accountId"));
this.getUser();
}
}
} catch (HttpException e) {
//Commitment point 1:It's not easy to read, so I'm addicted to commenting like this
//ex is getAccount, getAccountId,Stored with getUser
if (this.ex != null) {
if(this.ex.getReason() == HttpException.Reason.TIMEOUT) {
Thread.sleep(1);
continue;
} else {
//Suspended due to other error
break;
}
}
}
if(this.body != null) {
//Stop processing because the user is taken
user = this.body;
break;
}
} else {
//Interrupted because logout failed
this.body = null;
break;
}
}
try {
//Commitment point 1:DRY? what is that?
this.getAccountId();
if (this.body != null) {
this.getAccount();
if (this.body != null) {
getCache().setAccountId(this.body.get("accountId"));
this.getUser();
}
}
} catch (HttpException e) {
//ex is getAccount, getAccountId,Stored with getUser
if (this.ex != null) {
if(this.ex.getReason() == HttpException.Reason.TIMEOUT) {
Thread.sleep(1);
continue;
} else {
//Suspended due to other error
break;
}
}
}
if(this.body != null) {
//Stop processing because the user is taken
user = this.body;
}
break;
}
if(user != null) {
this.user = user;
//Commitment point 4:Not only login but also cache management
this.getCache().setUser(this.user);
}
return this.user;
}
//Commitment point 4:Method different from the original role
private Cache getCache() {
//Commitment point 2:Directly refer to the singleton
return Cache.getInstance();
}
//Commitment point 1:A non-getter method with a getter-like name
//Commitment point 4:Method different from the original role
private void getAccountId() throws HttpException {
try {
this.ex = null;
if(getCache().getAccountId() != null) {
//Commitment point 1:Reuse member variables in various ways
this.body = new HashMap<>();
this.body.put("accountId", getCache().getAccountId());
} else {
if(this.accessToken != null) {
//Commitment point 1:Reuse member variables in various ways
this.body = new HashMap<>();
this.body.set("token" , this.accessToken);
//Commitment point 2:Use of static methods&Call WebAPI directly
this.json = Http.createClient("https://account.example.com").post("/auth", this.mapper.serialize(body)).body();
this.body = this.mapper.deserialize(this.json);
}
}
} catch (HttpException e) {
this.body = null;
this.ex = e;
throw e;
}
}
//Commitment point 1:A non-getter method with a getter-like name
//Commitment point 4:Method different from the original role
private void getAccount() throws HttpException {
try {
this.ex = null;
//Commitment point 2:Use of static methods&Call WebAPI directly
this.json = Http.createClient("https://account.example.com").get("/accounts/" + this.body.get("accountId")).body();
this.body = this.mapper.deserialize(this.json);
} catch (HttpException e) {
this.body = null;
this.ex = e;
throw e;
}
}
//Commitment point 1:A non-getter method with a getter-like name
//Commitment point 4:Method different from the original role
private void getUser() throws HttpException {
try {
this.ex = null;
//Commitment point 2:Use of static methods&Call WebAPI directly
this.json = Http.createClient("https://example.com").post("/users", this.mapper.serialize(this.body)).body();
this.body = this.mapper.deserialize(this.json);
} catch (HttpException e) {
this.body = null;
this.ex = e;
throw e;
}
}
//Commitment point 4:Login Service, but you can also log out
public boolean logout() {
this.ex = null;
if (this.getCache().getUser() != null) {
this.user = this.getCache().getUser();
try {
//Commitment point 2:Use of static methods&Call WebAPI directly
Json json = Http.createClient("https://example.com").post("/logout", this.mapper.serialize(this.user)).body();
} catch (HttpException e) {
this.ex = e;
}
}
if (this.ex != null) {
this.user = null;
return false;
} else {
this.user = null;
//Commitment point 4:Not only log out but also delete the cache
Cache.getInstance().setUser(null);
Cache.getInstance().setAccountId(null);
Cache.getInstance().setToken(null);
return true;
}
}
}
I put on something that made my head flutter. If I was asked to test this, I would cry a little. It seems that I can climb to a higher height if I thoroughly implement the points I will write from now on, but I don't want anyone to climb too high and lose it, so I will leave it at this level this time.
Then, I will explain the "points that are difficult to test" that I was particular about.
The purpose of making a unit test is to confirm that it is "according to the specifications" as a major premise. Therefore, it is necessary to confirm the expected result with the body "I do not know what kind of processing is written in the code". If you don't, you'll fall into a test anti-pattern that simply says "** I'm just checking the code I've written and I'm not doing the tests I really need **".
However, in order to pass the unit test, it is necessary to know what kind of processing is done in the method to some extent. This is because the processes outside the class that are not the subject of the test are created while being mocked.
Therefore, it is quite difficult to make a test just by making readability worse. This time, I reduced the readability by the following method.
To add a little bit to the end, for example, getAccount () assumes that you are calling getAccountId (), and doing so alone will fail. Therefore, if you want to understand the process properly, you have to check the code here and there, which contributes to poor readability.
All the methods described in this LoginService are combined with the login () method. This is directly linked to the difficulty of testing.
To create a tight bond, we did the following:
By utilizing these, you can make the test difficult unless you forcibly make it into a Mock by using "PowerMock" etc. when testing. If it is a language that cannot be forcibly converted to Mock, there is no choice but to give up and allow Web API connection. However, in that case, the test will fail depending on the state because it depends on the data on the Web server side. I'm numb. ~~ Actually, I wanted to include control coupling etc., but I stopped because it was troublesome ~~.
As a result of using if statements etc. in vain, the cyclomatic complexity is about 20 (although it is not measured properly).
This means that you need to test at least 20 patterns, and in order to get the route right, you'll have to make a Mock like threading a needle. Having a private method also adds to the difficulty. You can see the madness at the time of creation.
This Login Service has all the processing required for login by itself, and the finish is very low in cohesion. Since the number of lines is still small now, it is awkward to call it a god class, but there is a possibility.
This Login Service has at least the following roles:
With these together, you just want to ** test the login flow **, but you have to consider WebAPI requests, logout logic, caches, and all sorts of ** noise **. lose. In addition, the high degree of coupling will reduce the SAN value when writing tests.
Next time, I'll try to make this code a little easier to test.
Recommended Posts