テストをリファクタリングで駄目にした話

Objective-C のテストをリファクタリングしたものの、結果的に使い勝手の悪いテストになってしまうことが何度かあった。

リファクタリング前

たとえば、以下のようなテストがあるとする。

- (void)testFileLog
{
    id formatter = [[Formatter alloc] init];

    LogMessage *message = [[LogMessage alloc] init];

    message.lineNumber = 10;

    message.filePath = @"";

    XCTAssertEqualObjects([formatter stringFromLogMessage:message], @":10",
                          @"file: %@", message.filePath);

    message.filePath = @"Hoge.m";

    XCTAssertEqualObjects([formatter stringFromLogMessage:message], @"Hoge.m:10",
                          @"file: %@", message.filePath);

    // ... 以下、同様のパターンでテストを羅列
}

同じようなコードの繰り返しで見通しが悪い。テストを追加するときは同じコードのコピペになるし、今後テストが増えていくとメンテナンスが大変になりそうだ。

リファクタリング (1)

パターンを見つけたら共通化したくなるのがプログラマ。別メソッドに切り出してみる。

- (void)testFileLog2
{
    [self doTestFileLogAtPath:@""
                       expect:@":10"];

    [self doTestFileLogAtPath:@"Hoge.m"
                       expect:@"Hoge.m:10"];

    // ... 以下、メソッド呼び出しの繰り返しでテストを羅列
}

- (void)doTestFileLogAtPath:(NSString *)filePath expect:(NSString *)result
{
    id formatter = [[Formatter alloc] init];

    LogMessage *message = [[LogMessage alloc] init];

    message.filePath   = filePath;
    message.lineNumber = 10;

    XCTAssertEqualObjects([formatter stringFromLogMessage:message], result,
                          @"file: %@", message.filePath);
}

LogMessage の初期化部分を doTestFileLogAtPath:lineNumber:expect: というメソッドに切り出し、テストではそのメソッドを呼び出すことで、かなり見通しがよくなった。

リファクタリング (2)

さらに、テストデータを分離することで、メソッド呼び出しの繰り返しをなくすことだってできる。

- (void)testFileLog3
{
    NSDictionary *spec = @{
                           @"" : @":10",
                           @"Hoge.m" : @"Hoge.m:10",

                           // ... 以下、入力と期待値の繰り返しでテストを羅列
                           };

    [spec enumerateKeysAndObjectsUsingBlock:^(NSString *filePath, NSString *expectedResult, BOOL *stop) {
        [self doTestFileLogAtPath:filePath
                           expect:expectedResult];
    }];
}

最初のバージョンに比べると格段に見通しがよくなった。テストの追加も、NSDictionary のエントリを追加するだけなので簡単だ。

隠れた問題

これらのリファクタリングは正当なものに思えるが、実は問題を抱えている。

テストが失敗したときのデバッグがやりづらいのだ。

• すべてのテストの XCTAssertXXX がひとつに集約されているため、Xcode のエラー表示が同じ行にいくつも表示されてしまう
• 失敗したテストだけをデバッガで追跡するとき、ブレークポイントに条件を指定するなどの工夫が必要になる

また、テストデータを NSDictionary に分離したバージョン testFileLog3 では、テストの分割が少々面倒くさい、という問題もある。一部のテストを別のテストメソッドに切り出したいときは、結局、testFileLog3 をコピペすることになるだろう。

リファクタリング (3)

では、どうするのがいいかというと、テストデータを分離するのではなく、テスト対象となるデータを生成する別メソッドを用意するのがよさそうだ。

- (void)testFileLog4
{
    XCTAssertEqualObjects([self formatAtPath:@""],
                          @":10");

    XCTAssertEqualObjects([self formatAtPath:@"Hoge.m"],
                          @"Hoge.m:10");

    // ... 以下、XCTAssertXXX の繰り返しでテストを羅列
}

- (NSString *)formatAtPath:(NSString *)filePath
{
    id formatter = [[Formatter alloc] init];

    message.filePath   = @"";
    message.lineNumber = 10;

    return [formatter stringFromLogMessage:message];
}

formatAtPath: のように、関心のあるテストデータのみを引数にとるヘルパーメソッドを用意し、testFileLog4 ではヘルパーメソッドの呼び出し結果と期待値を XCTAssertXXX でテストする。こうすることで、最初のコードよりも見通しがよくなるし、先にあげた問題も解決できる。

• Xcode のエラー表示は失敗した箇所ごとに表示されるため見やすい
• XCTAssertXXX の直前にブレークポイントを設定するだけで、失敗したテストのみをデバッガで追跡できる
• テストの分割も行単位の移動だけでできる

また、XCTAssertXXX の呼び出しを見れば テストの入力と期待値が一目瞭然 なのも見逃せないポイントだ。
このため、testFileLog4 ではそれまで指定していた XCTAssertXXX の第三引数以降を省略している。もちろん、入力と期待値からだけではテストの意図が分からないようなケースでは指定が必要だが、単純なテストではこれで十分だろう。

リファクタリング (おまけ)

さまざまな入力にたいして同じ結果を期待するようなテストなど、似たテスト同士は

• まとめて別のテストメソッドに切り出す
• XCTAssertXXX 呼び出しの共通部分をマクロとして定義

することで更に読みやすく、理解しやすいテストになる。

testFileLog4 に、結果が空文字列になるテストがいくつもあったとしよう。

- (void)testFileLog4
{
    // ...
    XCTAssertEqualObjects([self formatAtPath:@""],
                          @"");
    XCTAssertEqualObjects([self formatAtPath:@"."],
                          @"");
    XCTAssertEqualObjects([self formatAtPath:@".."],
                          @"");
    XCTAssertEqualObjects([self formatAtPath:@"./"],
                          @"");
    // ...
}

これを以下のように別メソッドに切り出し、テストメソッド名もテストの意図が分かりやすいものに変更する。

#define AssertFormatToBeEmpty(filePath) XCTAssertEqualObjects([self formatAtPath:(filePath)], @"");

- (void)testFileLogExpectToBeEmpty
{
    AssertFormatToBeEmpty(@"");
    AssertFormatToBeEmpty(@".");
    AssertFormatToBeEmpty(@"..");
    AssertFormatToBeEmpty(@"./");
}

段々 BDD っぽくなってきたが、このテストに特化したマクロを定義することで、デバッグのしやすさはそのままで読みやすくなった。
素の XCTest ではなく、specta などの BDD フレームワークでテストを書く場合でも、今回の指針は参考になると思う。