Lycolog

今回は何故単体テスト書くのかというのを個人的な見地から書いていく。TypeScriptとJestで記述するが、他の言語でも応用は可能だと思う。

単体テストとは何か？

関数やクラスのメソッドを単体としてみた場合の振る舞いを確かめるものである。以下はその一例だ。

hoge.ts

export const joinString = (string1: string, string2: string) => {
   return `${string1}${string2}`;
};

hoge.spec.ts

import { joinString } from '.';

describe('joinString', () => {
  it('引数として渡した文字列が正しく結合されること', () => {
    const actual = joinString('123', 'abcd');
    expect(actual).toBe('123abcd');
  });

  it('第一引数が空文字の時に第二引数のみが出力されること（空文字が結合されること）', () => {
    const actual = joinString('', 'edfg');
    expect(actual).toBe('edfg');
  });

  it('第二引数が空文字の時に第一引数のみが出力されること（空文字が結合されること）', () => {
    const actual = joinString('zxcvb', '');
    expect(actual).toBe('zxcvb');
  });
});

何のために単体テストを実装するか

• 実装されているコードが実装者の意図通りに動いていることを証明するため
  • いわゆるテストコードが仕様書になるってやつです
• 実装コードの改修時に元々の意図と異なる状況（不具合など）が発生した場合に、それを検知するため
• 初回実装時であってもコードを直したときに予期せぬ不具合を防ぐため
  • 動作確認した後にちょろっと直すことはよくあると思うが、そういう時に動作を担保するのに使える
  • 他にも実装時にコードを思案しながら書いているときに、動作確認をする手間を省くのにも使える

何故単体テストなのか？

結合テストやe2eテストではなく、何故単体テストをするか、これは実務面では結構出てくる疑問だと思うので、以下で説明する。

まず単体テストは一般的に結合テストやe2eテストに比べ、実行速度が速く、コードを修正したときに素早く回し、デグレードしていないかの確認に使うときに有益である。これは単体テストは実際にサーバーを起動したり、ヘッドレスブラウザでUI操作を行うなどしないためだ。他にも同じ処理を何度も見るのでテストのオーバーヘッドが膨らみ、実行時間が落ちるといったこともある。単体テストであれば簡単なロジックの実行しかしないし、結合部分はすべてモックにするため高速に実行できる利点がある。

次に結合テストやe2eテストは共通処理を一個直すと、それを使っている処理のテストをすべて直す必要があり、保守性がよくない。単体テストは基本的に直した箇所のテストコードを直すだけなので保守性が高い。

最後に単体テストでは基本的に関数単体を見るためテストコードが肥大化しづらく、見通しが良いので何をテストしているのかが結合テストやe2eテストと比べて分かりやすい。またスコープが関数単体であるため、カバレッジが取りやすく、いわゆるC0, C1, C2と言われる、命令網羅、分岐網羅、条件網羅の三点を網羅しやすい。結合やe2eでは共通処理の全機能を使うとは限らないため、この網羅がしづらい。

単体テスト向きの設計は何か？

これについてはSOLID原則を使った設計が一番無難だと考える。画面であれば更にMVCモデルを使うのがよい。

SOLIDにする利点

• S：単一責任の原則で関数の責務を切り分けることで、関数の責務を小さくすることができる。責務が小さい関数はテストケースが少なく、テストコードを書きやすく保守しやすい
• O：閉鎖解放の原則により、関数を呼ぶ関数自体を不変にでき、テストの保守性を向上することができる
• L：特にテストには貢献しない
• I：特にテストには貢献しない
• D：依存逆転の法則により、関数内で呼び出す関数をモックやスパイといったテストオブジェクトに置換できるため、テスト容易性が向上する

MVCにする利点

基本的に画面の実装は密結合になりやすい。密結合になっているとテストが書きづらいのでModelとViewとControllerに分けてテストをしやすくするのが目的だ。きちんと分けた場合、Viewは文字列が出ているか、表示項目が出ているかみたいなテストだけでよく、Modelはビジネスロジックのテストをするだけでいいし、Controllerは互いの橋渡しができているかを見ればいい。このため、テストコードをシンプルにできるのが利点だ。

欠点は画面の項目数が多い場合、インターフェースがそれに応じて肥大化するため、画面に仕様変更が入ったときに保守性が悪くなることだ。ここは品質とのトレードオフで許容するしかないと思う。

単体テストで処理の結合部をどう見るか

一つの関数で完結している関数はそれでいいとして、他の関数を呼んでいる関数をどのように単体テストするかという部分。よくあるパターンとしては他の関数ごとテストしてしまうことだが、これでは結合観点になってしまう。

この部分については呼び出す関数をモックすることで単体観点で確認することが出来る。呼び出す先の関数が単体テストで網羅されていれば、その関数の振る舞いは正しいはずなので、呼び出す側としては呼び出す関数をモックして、関数が正常終了したケースと、異常終了したケースでカバレッジが取れるようにテストすればよい。これは例えば次のようなテストを書くことが出来る。

index.ts

import { isEmpty } from './lib/string-util';

export const validate = (hoge: string) => {
  if (isEmpty(hoge)) {
    throw new Error('空です。');
  } else {
    // 何もしない
  }
};

index.spec.ts

import { validate } from '.';
import * as empty from './lib/string-util';

jest.mock('./lib/string-util');

describe('validate', () => {
  it('hogeが空文字の場合、例外がスローされること', () => {
    jest.spyOn(empty, 'isEmpty').mockReturnValueOnce(true);

    expect(() => {
      validate('aaaa');
    }).toThrow(new Error('空です。'));
  });

  it('hogeが空文字でない場合、例外がスローされないこと', () => {
    jest.spyOn(empty, 'isEmpty').mockReturnValueOnce(false);

    expect(() => {
      validate('aaaa');
    }).not.toThrow(new Error('空です。'));
  });
});

このテストではisEmpty()の実際の振る舞いを一切見ていないため、validate()の単体テストということが出来る。但しisEmpty()のテストが漏れていると機能しないため、これをきちんと機能させる場合、呼び出し先の関数の単体テストも必須だ。

単体テストに出来ない事と、単体テストの意義

単体テストは単体を見るものなので、当たり前だが単体しか見ることが出来ない。結合観点やe2e観点はなく、単体テストが通っているからデグレやバグがないという通りはない。

基本的に単体テストは動作確認が人間がやり、カバレッジが網羅できており、テストしている内容も正しければ以後のテストを工数を削るためのものである。過去に行った動作確認でテストが通っていて、今も通っているのだから恐らく動作しているだろうということを見るためのもので、単体テストが通っているからデグレがないとか、そんなことの保証はない。

では単体テストを何のためにするかだが、複数の確認工程鵜を作ることでバグを作りこむことを防ぐ役割が一つだ。例えば単体テスト＋結合テスト＋e2eテスト＋人による動作確認やテストと言ったように多層の防御があれば、不具合が起きる可能性を減らすことが出来る。もう一つはコードの仕様となることだ。実装を見て意図が読み取れないコードであってもテストコードがあればわかることもあるだろう。つまるところ実装品質を保つための防御機構の一つとみなすことが出来る。

単体テストを書くときにやったほうがいいこと

テストコードの動作確認をする

テストコードを書いても、そのテストコードが有効に動いているかどうかの観点が抜けていると有意なものとならないので、テストコードの動作確認は必要だ。偶にテストコードは動いているが、偶々テストがパスしてそれっぽく動いているだけみたいなこともある。TDDでやってもこれは起きえる。

期待値や結果値をいじって落ちるかどうかや、パラメーターなどの条件をいじって落ちるかは見たほうが良い。何をしてもパスする場合、そのテストコードは無意味である。

境界値テストを書く

文字列の入力パターンを見るテストや組み合わせテストではカバレッジを網羅していてもテストケースが漏れていることがある。例えば次のテストコードはカバレッジ100%だがtrueになる場合のテストが不足している。もしも関数の中身が必ずfalseを返す実装になっている場合に、これを確かめることが出来ない。実装を知っているからこれでいいとみなすのではなく、基本的には実装を知らない体で書いた方が良い。とはいえ、限度はあるので現実的に起きうる条件でテストするのが無難だろう。この辺りはSIerで堅めの開発をしていると身に付きやすいのだが、個人の感性に左右される部分であり難しいところでもあると思う。

index.ts

export const isEmpty = (str: string) => {
  return str === '';
};

index.spec.ts

import { isEmpty } from '.';

describe('isEmpty', () => {
  it('空文字でない時にfalseが返る事', () => {
    const actual = isEmpty('aa');
    expect(actual).toBe(false);
  });
});

変数を型で縛る

例えば以下の実装であれば引数は文字列型なので文字列以外が来ることは考えなくていい。

export const isEmpty = (str: string) => {
  return str === '';
};

しかし以下の実装では何が来るかわからない。テストとしてはありとあらゆる値が来ることを想定して書かないとリファクタリングに耐えられないだろう。そんなテストは書きたくない。そもそも実装からどれほどテストを書けばいいのか予測し辛いのもある。

export const isEmpty = (str: any) => {
  return str === '';
};

もしどうしても何が来るかわからない場合はunknownにしておくとよいだろう。何故なら以下のコードはエラーになるからだ。

export const isEmpty = (str: unknown) => {
  return str === '';
};

また全体的にきちんと型で縛っておくと、改修などで型が変わった時に実装コードやテストで型不整合が発生する機会が増え、影響範囲が分かりやすくなるため品質に寄与すると言える。

他に型が嘘をついている状態も良くないので、そのような状況がある場合は見直すのも一つだ。例えばnumber| stringが来るのに型定義はstringで、処理中で暗黙の型変換をしているとかがあると、危ういコードになりがちだ。

ここ数年見かける有効期限付きのURLの名前をPrivate Cache Distributionパターンというらしい。

これはFantiaみたいな課金サービスを利用している人にはもはや当たり前だと思うが、ページを表示してから一定時間たつと画像が表示できなくなるが、一定時間内であればそのURLは誰でも見れるみたいなやつだ。要するにURLに短めの有効期限を付けて認証されたユーザーにしか見えなくする仕組みのことだ。

参考

• CDP:Private Cache Distributionパターン

TypeScriptで関数を書いているときに戻り値の型を書くケースがあるが、個人的にはあれは基本書かないほうがいいと思っているので、その理由を書いていく。

コード記述が冗長になる

まず型を書くと記述が冗長になる。以下のコードを見ると戻り値型が長く読みづらく、書くのも面倒だ。まずこんな長い命名をやめたほうが…というのはあれど、現実問題として長い命名は存在するので仕方ない。

export const getCampanyAndDepartmentAndEmployeeFromPrefectureCode = (
  param: VeryveryLoooongestParamTypeNaming,
): VeryveryLoooongestReturnTypeNaming => {
  return param.db.con.execQuery(
    "SELECT * FROM foo WHERE id = :?",
    param.dbParam.foo.id,
  );
};

しかし以下であれば戻り値型がない分すっきりしていて見やすいし、書く手間も掛からない。更に型推論によって正しい型が返るので理想的だ。

export const getCampanyAndDepartmentAndEmployeeFromPrefectureCode = (
  param: VeryveryLoooongestParamTypeNaming,
) => {
  return param.db.con.execQuery(
    "SELECT * FROM foo WHERE id = :?",
    param.dbParam.foo.id,
  );
};

実装と異なる戻り値型を暗黙的に記述できる

例えば以下のように書けば戻り値の型は'foo' | 'not foo'となり、常に正しい型が返る。

export const foo = (isFoo: boolean) => {
  return isFoo ? 'foo' : 'not foo';
};

しかし以下のように戻り値の型を定義すると実装上存在しない'bar'が返る。これは実装時に無用な混乱を生む。一般的にこのようなケースは仕様削除やリファクタなどで生まれることがあると思うが、そういうメンテ漏れにもなるので書かないほうが安全だといえる。

export const foo2 = (isFoo: boolean): 'foo' | 'not foo' | 'bar' => {
  return isFoo ? 'foo' : 'not foo';
};

他にも次のケースでは戻り値がstringとなり、何が返ってくるのかが実装を見ないと解らなくなる。特に理由がないなら書かないほうがよい。

// 'foo' | 'not foo'になるはずだがstring扱いになる
export const foo3 = (isFoo: boolean): string => {
  return isFoo ? 'foo' : 'not foo';
};

型記述が混乱する

ここは基本的に前項の内容と重複する内容となる。

例えば次の二つの実装は同じだが、戻り値の型だけが異なる。こういう実装が混在すると実装の一貫性が失われ無用な混乱を生むので、指定しないほうが望ましい。

const IndexPage = (): JSX.Element => {
  return (
    <Layout title={'Hello Next.js'}>
      <>
        <h1>Hello Next.js 👋</h1>
      </>
    </Layout>
  );
};

const IndexPage2 = (): ReactElement => {
  return (
    <Layout title={'Hello Next.js'}>
      <>
        <h1>Hello Next.js 👋</h1>
      </>
    </Layout>
  );
};

書いてもよいと思うケース

例えば依存関係を持たせたい時など、インターフェースとして型を共通化したい場合は書いてもよいと思う。これはどこでそれを使うのかが自明になるからだ。改修時にも型によって関連処理が見出しやすくなるので意識しやすくなる。

export const createPostMessage = (
  channel: string,
  username: string,
  message: string,
): PostMessage => {
  return {
    channel,
    username,
    message,
  };
};

export const postMessage = async (param: PostMessage) => {
  try {
    return await fetch('https://example.com/api/postMessage', param);
  } catch (err) {
    return err;
  }
};

但しこのようなケースではUnit Testを書いて、実装された戻り値型を満たす値が返ることを確認するのが望ましい。

戻り値型を書かないことによるデメリット

TypeScriptの公式リポジトリによると、型推論の速度に悪影響を及ぼすとあるので、型推論の速度が落ちるという点だ。

もし型推論の速度が非常に遅いと感じた場合は書いてみてもよいと思うが、公式でも以下のように案内があり、和訳すると「型推論は非常に便利なので、これを普遍的に行う必要はありませんが、コードの遅いセクションを特定した場合に試してみると便利です。」とあるので、余程複雑なことをしていない限り不要だとは思うし、そんな複雑な型を返すような処理は必要がなければ書かないほうがいいだろう。

Type inference is very convenient, so there's no need to do this universally - however, it can be a useful thing to try if you've identified a slow section of your code.

少なくとも私は実務上、型推論の速度に困ったことがないのと、tscでビルドすることも稀であるため、ビルドに影響することもない。よって基本書いていない。

今の職場でコードを書いていてifに対して原則elseを書かないという雰囲気があるのですが、個人的にはelseを書きたいのでその話です。

世間的にelseを書かないというコーディングルールは一定の支持があるようで、まぁ一種の宗教だとは思っていますが、個人的にはどうなんかなと思っています。まぁ割と最近の流行りな気もしていますが…。（昔はなかったと思う…というとアレですが）

elseがあってほしい理由

else ifやelseブロックがあると処理の関連性が見やすくなると考えています。

処理の関連性がわかりやすくなる

例えば以下のコードの場合、ifが連続しているよりelseがある方が処理の関連性が明示的に見えると思います。elseで繋げないことは基本的に処理として関連性がないはずです。

// ifのみ
export const hoge = (mode: 1|2|3) => {
  if (mode === 1) {
    // なんかの処理
    return 'hoge';
  }
  if (mode === 2) {
    // なんかの処理
    return 'piyo';
  }
  if (mode === 3) {
    // なんかの処理
    return 'fuga';
  }
}

// elseあり
export const piyo = (mode: 1|2|3) => {
  if (mode === 1) {
    // なんかの処理
    return 'hoge';
  } else if (mode === 2) {
    // なんかの処理
    return 'piyo';
  } else if (mode === 3) {
    // なんかの処理
    return 'fuga';
  }
}

予期せぬ不具合が入り込みづらくなる

ifだけで構成されたコードでは以下のようにifブロックの外に処理を入れ込むことができますが、このときA1, B1, C1のifの外にある処理は後続処理に影響します。しかしこの書き方だと処理の影響範囲が見積もりづらく、不必要なバグを生む元になるため、個人的には避けたいと考えています。（他の処理の作用に引っ張られているので、副作用のある実装ということになると思っています）

またA1, B1, C1の行数が伸びるとコードの見通しが悪くなり、if同士の関係性が分かりづらくなります。

elseを使うとこういった副作用的な振る舞いをする実装が作りづらくなり、影響範囲もブロックインデントで分かれるので、そういった心配がなくなります。

// ifのみ
export const hoge = (mode: 1|2|3) => {
  // なんかの処理 A1
  if (mode === 1) {
    // なんかの処理 A2
    return 'hoge';
  }
  // なんかの処理 B1
  if (mode === 2) {
    // なんかの処理 B2
    return 'piyo';
  }
  // なんかの処理 C1
  if (mode === 3) {
    // なんかの処理 C2
    return 'fuga';
  }
}

elseが必要なのに書き忘れることが減る

elseを書かないことに捕らわれていると以下のようなコードが生まれることがあります。

さてこのコードにおいて!validHoge(hoge) || !hasPiyoのケースはどのように処理されるのでしょうか？答えは処理されませんが、もしこのコードに対してUnit testが実装されておらず、特段のドキュメントもなければ、それが正しいのかどうかをコードから読み取ることが出来ません。

const isValidUsername = (username: string, hasInputed: boolean) => {
  if (validUsername(username) && hasInputed) {
    return requestSearchUsername(username);
  }
}

せめてこう書いてあれば判断も付くでしょう。

const isValidUsername = (username: string, hasInputed: boolean) => {
  if (validUsername(username) && hasInputed) {
    return requestSearchUsername(username);
  } else {
    // 何もしない
  }
}

因みにこの処理はユーザー登録フォームでユーザー名の書式が正しければ既に存在するユーザー名かどうかをAPIに問い合わせ、既に存在すればエラーメッセージを、存在しなければtrueを返すという内容ですが、そもそも書式が不正である場合は何もしないため、ユーザーはエラーであることを知ることが出来ません。

もしelseブロックを書いていれば考慮漏れに気づけた可能性もあったのではないかと思います。

elseを使うケースを考えなくて良くなる

ifに対して原則elseを書かないというルールがあってもelseを書かないと成り立たないケースは存在します。そう言った場合に原則ifしか書いてはならないというルールがあるとelseを書いてよいかどうか考える必要が出てきますが、元からelseも書いて良いルールであればそこは考える必要がありません。個人的に判断の余地が生じるコーディングルールはチーム開発ではない方が良いと考えています。

またこのルールの結果、本来必要だったelseを書かなかったことにより不具合が発生する可能性もあります。（実装者が軽率にelseを使わなかったことによる判断ミス）

そもそも何故elseを書いてはならないのでしょうか？コードのネストが深くなるからでしょうか？少なくともelse if相当のifには、その作用はありません。あるとしたら純粋なelseブロックはインデントが減るでしょう。しかしこのインデントはあったほうが処理の関連性が掴みやすくなると思います。

elseのないifは悪か？

ここまで散々elseを書くべきと言ってきましたが、ではelseがないifは悪かと言うと、そうは思いません。例えば、以下のような早期リターンと呼ばれる記法であればそれは良いと考えています。では何を早期リターンとするかですが、個人的には例外的に処理を継続しない場合を一つの基準にしています。私もここの感覚は上手く言語化できていないのですが、恐らく何もしないときだけelseを書かないのは問題ないと考えています。

これは本質的ではない処理のせいで、いたずらにネストが増えるのを防ぐためです。関連性がある場合にelseを使うのがコードのメリハリとして目視で読んだときの認知性の向上に繋がると考えています。

export const putLog = (message: string) => {
  if (message === '') return;

  console.log(message);
}

この点については個人的に共感できる意見があったので以下の記事を紹介させて頂きます。

else句を使わないのが良いコードなの？いや、そんなはずは・・・ · DQNEO日記

記事中にある以下の部分、特殊ケースに対してガード節を使うというところで、例外ケースでのみ早期リターンをするという部分がありますが、やはりこれが一番無難だなと思いました。

条件記述の単純化　「ガード節による入れ子条件記述の置き換え」
特殊ケースに対してガード説を使う

ただ例外ケースとは何か？それは数値化でき、チーム開発で標準的に取り扱えるものなのか？と言われると、正直私もそこまでは言語化できていないので難しい部分ではあります。そもそも人間が書くコードが完全に均一になることはないと考えているので、そこまで強い思想を持って考えてはいないです。（もしChatGPTに書かせてもバラツキは出るでしょう）

私がこの記事で言いたいのは単にelseを書いてもいいのではないかということと、その場合でも早期リターンは許容しても問題ないだろうというところなので、一旦早期リターンの基準についてはここでは考えないこととします。

そうなると結局elseなしでifだけあればいいじゃないかという話に戻ってくるとは思うのですが、正直そこは常識で考えてほしいというところです。まぁ常識も人の数だけあるので難しいですし、そんな物があればこんな記事も生まれていないわけで、ソフトウェア開発は難しいですね。

undefinedの判定方法が複数あるということでundefined判定の処理速度比較をしてみたのでその結果。

端的に言うと、hoge === undefinedとtypeof hoge === 'undefined'の二方式がある。後者は原則考慮不要だが、言語仕様上存在しているので比較したが、現実的に見た場合、どちらで記述した場合でも処理速度に有意な差はないように感じた。

確認環境

比較結果

hoge === undefinedの方が早く見えるが実行するタイミングで変わるので誤差の範疇だと思う。

確認コード

const tyof = (param?: string) => {
  return typeof param === 'undefined';
};

const undef = (param?: string) => {
  return param === undefined;
};

const tyStart = +new Date();
for (let i = 0; i < 10000000000; i++) {
  tyof();
}
console.log('typeof', +new Date() - tyStart);

const unStart = +new Date();
for (let i = 0; i < 10000000000; i++) {
  undef();
}
console.log('undefined', +new Date() - unStart);

TSから生成されたJS

"use strict";
Object.defineProperty(exports, "__esModule", {
    value: true
});
const tyof = (param)=>{
    return typeof param === 'undefined';
};
const undef = (param)=>{
    return param === undefined;
};
const tyStart = +new Date();
for(let i = 0; i < 10000000000; i++){
    tyof();
}
console.log('typeof', +new Date() - tyStart);
const unStart = +new Date();
for(let i = 0; i < 10000000000; i++){
    undef();
}
console.log('undefined', +new Date() - unStart);

あとがき

MDNを読む限りtypeof hoge === 'undefined'は該当変数が存在しない場合に有用なようであるが、TypeScriptで書いている場合、通常このようなコードが生まれることがなく、仮に起きるとした場合、次のようなコードになるため現実的に考慮する必要はない。なおMDNにも「こんなことはしないこと」と書いてあるので、一般的なコードでないことは客観的にも伺えるだろう。

(() => {
  const undefined = 123;
  const hoge = undefined;

  if (typeof hoge === 'undefined') {
    console.log('hoge is undefined');
  } else {
    console.log('hoge is not undefined');
  }
})();

上記コードの実行結果としてはhoge is not undefinedが出力される。

このコードの主な問題点

1. const undefined = 123;というコードは予約語を変数名にしているため、混乱を招くコードであり、書かないことが好ましい
   1. MDNには予約語ではないとあるが、一般的には予約語の一つとして解釈して支障ないと考える
2. このコードはESLintのeslint:recommendedで検知されるため、通常であれば書かれることはない
   1. no-shadow-restricted-namesに引っかかる

なお、このコードは例示のために即時実行関数形式で記述しているが、必要がない限りこの形式での実装は避けたほうが問題が少なくなると思う。これは不必要なネストが生まれたり、スコープの混乱を生むためである。