初めまして。Qiita初投稿になります。h0ng0yut0です。よろしくお願い致します。
今回は、「テストコードを粛々とリファクタリングした話」を書かせていただきます。
（※ソースコード等はこの記事以外に使用されておりません。）
###読んでほしい方
- 僕と同じく、テストコード触りたてで、これから頑張っていこうと思っている方
- チームで導入してみたものの、なんか突っ走りすぎちゃってる気がしている人
#テストコードを粛々とリファクタリングした話
##チームに参加した直後のテストコード
UserCommentDaoTest
// ーーーーもろもろインポートーーーー
@RunWith(Seasar2.class)
public class UserCommentDaoTest {
UserCommentDao userCommentDao = new UserCommentDao();
/**
* テストの準備
*/
TestContext ctx;
public void before(){
ctx.setPreparationType(PreparationType.ALL_REPLACE);
}
public void testGetUserCommentList() {
//変数
String userId = null;
String from = null;
String to = null;
String comment = null;
String orderByItem = null;
int limit = 50;
int page = 1;
List<UserCommentListDto> result = new ArrayList<UserCommentListDto>();
//正常系：userId1のコメントを取得
userId = "1";
from = null;
to = null;
comment = null;
orderByItem = "createdAt";
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(6));
//正常系：期間指定でuserId1のコメントを取得
userId = "1";
from = "2014-11-01 00:00:00";
to = "2014-11-05 00:00:00";
orderByItem = "createdAt ASC";
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(6));
assertNotSame(result.get(0).userId, result.get(1).userId);
//正常系：コメント(部分一致)でuserId1のコメントを取得
userId = "1";
comment = "サンプルコメント";
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(1));
//正常系：フィルタリング結果なし
userId = "1";
comment = "サンポーコメンツ";
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(0));
//異常系：存在しないuserId
userId = "999";
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(userId, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(0));
//異常系：userIdがnull
result = new ArrayList<UserCommentListDto>();
result = userCommentDao.getUserCommentList(null, from, to, comment, orderByItem, limit, page);
assertThat(result.size(), is(0));
}
// ----こんな感じで、メソッドごとにたくさんのアサーション等が…----
}
１メソッドに対して１テストメソッドが用意されており、その中に複数のテストケース、複数のアサーションが混在している形でした。
##含まれているアンチパターンについて
| アンチパターン | このコードにおいて | なにが困るの？ |
|---|---|---|
| 嘘つき | 嘘つきというか、取得してきたresultのサイズでのアサーションとなっており、検索条件に合致したデータを取得できているのかというアサーションが存在しない。 | 正しくそのメソッドが動作していることを保証したテストコードにはなっていないので、テストコードの意味自体がとても薄れてしまう。 |
| タダ乗り | タダ乗りというか、テストケースごとにテストメソッドを書いておらず、メソッドの全機能を１つのテストメソッドでチェックしようとしている。 | 途中のアサーションで予期せぬ結果が出た場合に以降のアサーションは判断されないので、どんなテストケースで失敗しているのかを１度のユニットテスト実行で判断できない。 |
| のろまな動き | どうやら、ユニットテストを回す際に、CSVデータをmysqlに毎回つっこんでテストをしているらしい | DB繋ぐ回数が多いと、テスト遅くなるよね。 |
##修正の方針
- 嘘つき…アサーションをできるだけ正しい形にする。
- タダ乗り…テストケースごとにテストメソッドを作成し、どのテストケースで失敗しているのかをユニットテスト実行時にわかりやすくする。
- のろまな動き…なんとかDB繋がずにやろう。
##リファクタリング後のテストコード
UserCommentDaoTest
@RunWith(Enclosed.class)
public class UserCommentDaoTest {
UserCommentDao userCommentDao = new UserCommentDao();
private static void initializeDaoTest() throws Exception {
TestHelper.preparingData(userCommentDao, new String[] { "user", "user_comment" });
}
static {
try {
initializeDaoTest();
} catch (Exception e) {
e.printStackTrace();
fail(e.toString());
}
}
@RunWith(Seasar2.class)
public static class testGetUserCommentList {
String userId;
String from;
String to;
String searchComment;
String orderByItem;
int limit;
int page;
List<UserCommentListDto> result;
@Before
public void before() {
userId = "1";
from = null;
to = null;
searchComment = null;
orderByItem = "createdAt";
limit = 50;
page = 1;
result = new ArrayList<UserCommentListDto>();
}
@Test
public void userId1のコメントを取得() {
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
assertThat(result.size(), is(not(0)));
for (UserCommentListDto comment : results) {
assertThat(comment.userId, is(userId));
}
}
@Test
public void 期間指定でuserId1のコメントを取得() {
from = "2014-11-01 00:00:00";
to = "2014-11-05 00:00:00";
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
assertThat(result.size(), is(not(0)));
for (UserCommentListDto comment : results) {
createdTimeMills = comment.createdAt.getTime();
fromTimeMills = from.getTime();
toTimeMills = to.getTime();
assertThat(createdTimeMills, greaterThanOrEqualTo(fromTimeMills));
assertThat(createdTimeMills, lessThanOrEqualTo(toTimeMills));
}
}
@Test
public void コメントの部分一致でuserId1のコメントを取得() {
searchComment = "サンプルコメント";
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
assertThat(results.size(), is(not(0)));
for (UserCommentListDto comment : results) {
assertThat(comment.commentText.contains(searchComment), is(true));
}
}
@Test
public void コメントの部分一致しない場合() {
searchComment = "サンポーコメンツ";
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
assertThat(result.size(), is(0));
}
@Test
public void 存在しないuserIdの場合() {
userId = "999";
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
assertThat(result.size(), is(0));
}
@Test(expected = Exception.class)
public void userIdがnullの場合() {
userId = null;
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
}
}
// ----他、同じように修正----
}
##リファクタリングの解説
アンチパターンの解消に関して
| アンチパターン | このコードにおいて | 利用したもの |
|---|---|---|
| 嘘つき | 様々なMatcher、様々なJavaのメソッドを調査して、しっかりとテストケースに合ったアサーションに書き換えた。 | Matcher等 |
| タダ乗り | 構造化を行うことで、１つのメソッドに対して複数のテストケースとそれに対応したテストメソッドを記述した。（xSpec系等でよく言われているdescriveとcontextのような理想の形とは違うののの、落とし所だとは思っている。） | Enclosed |
| のろまな動き | 今回のソースには載せていないが、jdbcManagerの接続先をh2dbのmysqlモードに変更して対応している。（ついでにユニットテストに使うデータとローカル環境構築でmigrateするときに用意するデータの二元管理されていて、なにかと面倒くさいので一元管理に変更した。） | "先輩"の記事のリンク |
###その他
なぜstaticイニシャライザなのか
デバッガとかSystem.out埋めての調査でアレですが、複数ファイルを同時にユニットテストを実行する場合、BeforeClassアノテーションをつけたメソッドが１番初めに呼ばれたBeforeClassアノテーションをつけたメソッドしか動作しなかったので。
テストスイートを作成しました
Enclosedを使用して構造化したのですが、テストメソッドが２重に実行されてしまう問題が起こったため。
テストコードをDRYにしすぎると、可読性が下がると言われていますが…
おそらくsetUpやtearDownのような仕組みを使ったり、独自メソッドを使ってDRYにすることはあると思います。
僕の場合よくやるのは、以下のような手法です。
- setUpに正常系の場合の変数の値をセットする。
- テストメソッド内で、テストケースに関わる変数に関して代入を行い、テスト対象のメソッドを実行している行と近い状態にしておく。
例.userIdがnullの場合
// Beforeアノテーションをつけたメソッドに正常系を用意
@Test(expected = Exception.class)
public void userIdがnullの場合() { // ← テストケース
userId = null; // ← テストケースに沿ったデータの用意
result = userCommentDao.getUserCommentList(userId, from, to, searchComment, orderByItem, limit, page);
}
#所感
結構ありがちなのが、『動いているプロダクションコードをあまり触りたくない』という感情だと思います。動いているプロダクションコードに触れる恐怖や不安を取り除く１つのツールがテストコードだとおもいます。そのツールを使いこなせるようになり、不安がなくなったときに「プロダクションコードをよりよい状態」にfearlessに変更できますね。
今回は、「プロダクションコードをよりよい状態」にするための準備の準備みたいなお話でした。
準備はできたので、これからレガシーコードに少しづつ挑んでいきます。
#読んでいただいた方へ感謝
長い記事を読んでいただきありがとうございます。まだ新卒で入社してキャリア１年ほどのエンジニアなので、色々とアドバイス等いただけると嬉しいです！
http://qiita.com/h0ng0yut0
#参考サイト
「TDDのアンチパターン」
http://www.hyuki.com/yukiwiki/wiki.cgi?TddAntiPatterns
#追記
このQiitaの記事を書いた後、チームで「Light Lightning Talk（軽めのライトニングトーク）」として、以下リンクのスライドと共にチームメンバーにテストコードに関して共有いたしました。
Qiita記事をトリガにして、社内LTとかを文化にしてみても面白いと思いますヽ(´ー｀)ノ