はじめに

※ (2024/03/14 16:33) 「インテグレーションテストの気軽な実行・変更ができない」節にて、データのクリーンアップを teardownで行うよう修正

EC開発-B グループの岡崎と EC開発-A グループの菊川です。2人とも普段は MonotaRO の EC サイトの開発に従事しています。

今回は、昨年11月に開催した、テストとリファクタリングのためのワークショップの中で行ったライブコーディングの準備をするにあたって困ったことについて記載します。

ライブコーディングでは、参加者全員の前で実際のプロダクトのソースコードをリファクタリングする、ということにし、それにあたって研修の運営メンバーでリファクタリングに取り組んでみました。ただ闇雲にリファクタリングするのではなく、研修では参加者に「どのような流れや考え方でリファクタリングをするか」を理解してもらえるように、運営メンバーで認識を合わせながら、また、方針を整理しながらリファクタリングを進めました。その過程において、実際にプロダクトコードをリファクタリングしていく際にもぶち当たるであろう壁や課題を解決していきました。

今回行ったことの多くは、リファクタリングそのものではなく、リファクタリングを実施するにあたっての準備や環境整備でした。リファクタリングの対象がすでに多くのお客様に毎日のように利用していただいているサービスとして動いているソースコードであることから、既存の機能を壊さないよう安全にリファクタリングを進める必要がありました。そのためには、信頼できる自動テストが整備されており、ソースコードを変更したらすぐに試せる環境が必要でした。また、リファクタリングを行うために使えるリソースも限られているため、いかに効率的に進めるかも考えねばなりません。世の中に紹介されている様々な手法の中から、自分たちの事情に合わせてよりコストパフォーマンスの良いものを選び、進めていくことが重要でした。

この記事は、今回実際に行ったリファクタリングの中身を詳細に紹介・解説する、というものではなく、前述のようなことを運営メンバーで行ってきた議論や実際のコーディングの過程で、困ったことをどう解決していったかをご紹介します。おそらく、リファクタリングをしようと意気込んでいる人の多くが通る道になるので、この記事の内容は参考にしていただけるのではないかと思います。

なお、このワークショップがどんなものであったかについてはすでに公開済みの記事「リファクタリングを文化にする〜組織が技術的負債と向き合うワークショップ〜」にてご紹介しておりますので、そちらも併せてお読みください。

はじめに
今回のリファクタリング対象についての簡単な紹介
ライブコーディングの題材をリファクタリングしたメンバー
リファクタリングを進めるにあたってぶつかった課題と克服
結論

今回のリファクタリング対象についての簡単な紹介

今回のワークショップでリファクタリングの対象としたのは MonotaRO の EC サイトを支える API のソースコードです。全て Python で実装されています。

この API の役割については過去の記事 Software Design連載 2021年9月号「テストが無い」からの脱却 - MonotaRO Tech Blog でも触れられていますので、ご参照ください。

API ではユニットテストとある程度のリグレッションテストが整備されています。

今回対象としたのは「郵便番号検索 API」です。これは、郵便番号情報を受け取り、DB に保存された住所情報を返す API です。

このエンドポイントをリファクタリング対象としたのは以下の理由からです：

特定のドメイン領域の知識がなくても理解できるエンドポイントであるため。
- 研修を受ける開発者は様々な業務背景があるため、前提となる知識はなるべく少なくしたかった。
本エンドポイントは API で典型的な処理内容・構造になっているため、他のエンドポイントでも同様のリファクタリングを適用しやすい。
- 典型的な処理内容：パラメータで指定された内容に応じて DB から値を取得してレスポンスの形に整形して返却する。
- 典型的な構造：処理内容によってある程度ソースコードが分かれているものの、1つの関数・1つのソースファイルに多くのことが詰め込まれている。

ライブコーディングの題材をリファクタリングしたメンバー

郵便番号検索 API のリファクタリングを進めていったのは、今回のソフトウェア研修の運営メンバーでした。

運営メンバーは日々 EC サイトの開発にはなんかしら携わっているものの、普段は別チームで業務をしています。

リファクタリングに関する知識や考えはある程度持っている人ばかりでしたが、認識を合わせて開発をしていくのはこのリファクタリングの場が初めてでした。誰か1人が進めていったり、誰かが司令塔となったりするのではなく、どのような方針で進めていくかを協議し、試行錯誤しながらリファクタリングを進めていきました。

リファクタリングを進めるにあたってぶつかった課題と克服

ここからは、リファクタリングを進めていく中で実際にぶつかった課題とそこで行われたメンバー間での議論について記載します。ライブコーディングではとても綺麗に進行したように見せましたが、その流れを作る上では紆余曲折があったんだな、ということが伝われば幸いです。

リファクタリングの方針を決める

ソースコードから見えた問題点

ある程度、リファクタリング対象のソースコードを決めたら、実際のリファクタリングにかかるべくソースコードをざっくりと眺めました。

その結果、メンバーで今回扱うソースコードの問題点を以下のように洗い出しました：

コードを見ただけでは辞書の各フィールドの存在とその型が分からない
- パラメータ、DBから取ったレコード、レスポンス
特定のクラスに処理を詰め込みすぎている。
- 単一責任の原則：Single responsibility principleに反する
- そのクラスでやっている内容：
  - APIのレスポンス生成
  - 郵便番号取得処理
  - Validation処理
  - パラメータの初期化：検索で利用する SQL に適した内容にパラメータを成形する
  - 配送不可地域を取得
DBアクセス方法が2パターンある：ソースコードにSQL直書きしているパターンとRecordSetオブジェクトを利用しているパターン

改善のアイデア

先ほども書いた通り、運営メンバーはある程度リファクタリングについて知識を有しているため、どのようなリファクタリングを行えば良いかについてはアイデアはいくつか挙がってきました。しかし、今回は時間が限られていたので、費用対効果が高いものに絞って実施することにしました。

案1：全体的にクリーンアーキテクチャ化する

今のコードは（一応）アーキテクチャとしては「こうすべき」というものが定められているのですが、一般的なものではなく厳格なものでもないため、書かれる箇所によって書き方がバラバラです。そのため、もはや全体的に書き直してしまった方が早いのではないか、という意見が出てきました。書き直すとなると、クリーンアーキテクチャを採用するとメンテナンスも向上しそうです。

しかし、全面的に書き直すとなると、既存のテスト（ユニットテスト、リグレッションテスト）を全て捨ててしまうことになります。アーキテクチャが大きく変われば、テストも一から書くことになります。すると、問題になってくるのは、リファクタリングを行ったが故にエラーが発生することを防ぐにはどうすれば良いか、ということです。前述の通り、ユニットテストとリグレッションテストはすでに整備されているものの、特にインテグレーションテストはケースが少なく、ふるまいが正しいことを担保するには有効なものとは言えませんでした（後にも触れます）。そこで、過去の記事（Software Design連載 2021年10月号スナップショットテストの可能性を追求する - MonotaRO Tech Blog ）でも触れたようなスナップショットテストの考え方を利用することを考えました。実際に本番環境で利用されているパラメータとそのレスポンスをモニターし、それによってE2Eテストを作る、といった具合です。

しかしながら、限られた時間の中で実施するには最後まで到達できる見込みがなかったことから、上記のような作業は膨大になりすぎるため、今回は見送ることにしました。

案2：ORM を全面的に利用するようにする

DB アクセスに関して、処理の書き方が 2 パターンあるということは先に書きましたが、これらを統一することを目指して O/R マッパー（ORM）を導入して利用する書き方にすれば良いのではないか、という意見が上がりました。エンドポイントが最初に実装された時代からは年数も経っており、 SQLAlchemy のようなライブラリを活用することで DB アクセス周りの処理の可読性を向上できそうです。

今回対象としていたエンドポイントが扱っていた SQL をみてみると、条件分岐を含む複雑な SQL 生成を行っていました。単純に SQLAlchemy を利用して書き換えると、実装の結果と妥当性の検証が大変になりそうでした。やはり今回限られた時間の中で実施するには困難であると判断しました。

案3：辞書のフィールドがわからない問題を解決する

今回扱うエンドポイントの箇所のみならず、色々な箇所で問題になっているものでした。異なるレイヤーのコードとのやりとりは基本的に辞書を用いて行われており、初めてソースコードをみただけでは、どのフィールド（キー）にどのような値が入るのかがわからない状態になっています。

このような問題を解決する最も良い方法は Python の標準機能に含まれている dataclass を利用する方法や、pydantic のようなライブラリを利用する方法が考えられます。しかし、そこまで大規模なことをせずとも、辞書のキーと値の対応を厳格にするという目的の達成のためには Python の標準機能である TypedDict を利用すれば良さそうでした。TypedDict は型ヒントですので、既存の処理内で違反をしていたからといって動作しなくなるということはありません。また、API の CI では mypy を利用した静的解析が導入されているため、 TypedDict を導入したリファクタリングを行うには効果的な環境が整っていると判断しました。

今回のリファクタリングでは、型ヒントを大いに活用し、辞書については TypedDict を積極的に使っていく方策を採用しました。

案4：責務の分離を行う

前述の通り、今回扱うエンドポイントのコードは、単一の関数/クラスに様々な処理を詰め込んだ結果、非常に読みにくいコードになってしまっています。ドメイン知識がそこまで必要ではないエンドポイントのコードとは言え、このような状況では、この API がどのようなふるまいをするのかを正確に把握することは困難でした。責務の分離を行うことを意識しながらソースコードを読むことで、このエンドポイントの振る舞いを把握することもできますし、可読性の高いソースコードに書き換えることもできます。

責務の分離を行う中での変更は、ちょこちょこと試しながらエラーがないかを確認しながら進めることができる（ダメでもすぐに戻って来れる）ので、実施するにあたってはそんなにコストはかからないという判断になりました。

大規模なリファクタリングに挑むにあたって、少しずつ地道に責務の分離に取り組む方針を採用しました。

方針の議論まとめ

運営メンバーで行った議論の結果をまとめると、以下のとおりです：

リファクタリングのアイデア	採用/不採用	理由
全体的にクリーンアーキテクチャ化する	不採用	クラス構造など大幅にソースコードを変更する必要があり、時間がかかりすぎるため。
ORMを使うようにする	不採用	SQL 直書きを止めたいのは山々だが、複雑な SQL を書いている箇所があり、これらを ORM で実現しようとすると時間がかかりそう（実装と結果の妥当性の検証の大変さ）であるため。
辞書のフィールドが分からない問題を解決する	採用	すでに CI で静的解析ツールの mypy が導入されていること、今回改修対象の Python のバージョンでは Typed Dict を利用することができることから、比較的簡単に導入できそうであるため。
責務の分離	採用	ふるまいを変えずにソースコードを分離していくだけならそこまでコストはかからないと判断したため。

以上の下調べと議論を経て、責務の分離と Typed Dict の導入から進めていくことに決めました。

1人でリファクタリングに取り組んだり、方針を立てたりすると、どうしてもどこかで仕入れた理想系の形を目指して、工数や規模を考慮しない形で進み始め、途中でうまくいかなくなり頓挫してしまうことが多々あるのではないかと思います。今回のように、複数人で対象のソースコードを読み、問題点を洗い出しながら、今回の条件に合うリファクタリングは何かを議論することで、無理なく目標も失うことなく進められるリファクタリングを開始できると感じました。さっさと手を動かしたくなるのがエンジニアの性（さが）ですが、最初に冷静に状況を分析することが重要であると思います。

前述の通り、集まったメンバーはある程度の知識は有しており、各々リファクタリングの考えを何かしら持ったメンバーでした。運営メンバーで熱い議論が交わされましたが、揉めるようなことは一切なく、いろんな案や意見を前向きに検討しながら方針を決めていくことができました。揉めずに議論できたのは、メンバー全員で目標を共有できていたこと、また、折に触れて「どのようなリファクタリングをすれば良いか」について、既存の概念を用いて言語化することができていたことが理由ではないかと思います。

題材として選んだエンドポイントの詳細な仕様を知らない

運営メンバーにはリファクタリングの対象として選んだ郵便番号取得エンドポイントの仕様について詳しいメンバーがいませんでした。そのため、手動テストによる担保では安全とは言えず、リファクタリングによって対象のエンドポイントのふるまいが変わっていないことを担保するにはどうすべきか考える必要性が有りました。

このような場合に取れるアプローチとして以下のような方法が考えられます。

仕様に詳しい人に協力を仰ぐ
残されたドキュメントを活用して仕様を把握する
既存のテストコードを活用してふるまいを担保する

上記の①のアプローチは人間の知識に頼っている分正確性に欠けます。また、ワークショップ運営業務に携わってもらうのに業務調整が必要となりオーバーヘッドが大きいです。また②のアプローチはそもそもドキュメントが残されているか分からないことに加え、仮にあったとしてもメンテナンスがされている可能性が低いため正確性に欠けることが考えられました。そのため、上記の3つの中で我々は③の「既存のテストコードを活用してふるまいを担保する」アプローチができないかを優先的に探ることにしました。

そこで運営メンバーで、対象のエンドポイントのテストコードについて調べました。すると、インテグレーションテストとユニットテストが存在することが分かりました。テストの内容を読んでみるとE2Eテストは異常系を1ケースのみで仕様の担保には使えなさそうでした。しかしながら幸運にもユニットテストではテストケースが30件ほどでカバレッジが100%だということが分かりました。カバレッジが100%であれば、このユニットテストに通すことでふるまいの担保ができそうです。

エンジニアなら仕様を熟知したメンバーがいないシステムのリファクタリング・機能追加をすることが求められる場面に出くわすことはそこそこあると思います。この場合は既存のアセットから活用できそうなものがないか探すことから始めることが最初の一手になると思います。今回で言えば幸運にもその一手がカバレッジ100%のユニットテストでした。

このユニットテストを使えばふるまいを担保しつつリファクタリングを進めていけそうにいったんは思えました。しかしながら、そう上手くことは運びませんでした。次節でこのユニットテストが抱えていた問題について説明します。

既存のユニットテストを活用できない

早速、リファクタを開始しようとしましたが、コードの変更を加えると、すぐにユニットテストが失敗する状態となりました。その理由を探るためユニットテストとテスト対象のコードを読むことにしました。以下にリファクタリング対象のエンドポイントのコードとユニットテストの一部を示します。

# テスト対象のコード
class ZipLoader:
    """住所情報操作"""

    def __init__(self):
        """コンストラクタ"""

    def get_body(self, param):
        """レスポンスを取得"""
        check = self.__check_param(param)  # パラメータのチェック処理  # (パラメータ)
        if not check:
            # NGの場合の処理
        self.__init_param(param)  # パラメータを初期化
        recs = self.__get_dm_zip(param["zip_info"])  # 住所情報を取得  # (郵便番号情報)
        result = self.__get_response(param["zip_info"], recs)  # レスポンスを作成  
        return body

    def __check_param(self, param):
        """パラメータのチェック"""

    def __init_param(self, param):
        """パラメータ値を初期化"""

    def __get_dm_zip(self, zip_info_list):
        """住所情報の取得"""

    def __get_response(self, zip_info_list, recs):
        """レスポンスの作成"""

# ユニットテストのコード
class ZipLoaderTest(unittest.TestCase):
    def setUp(self):
        self.loader = ZipLoader()

    def test_get_body_200(self):
        """get_body メソッドのテスト 正常終了の場合"""
        with mock.patch.object(
            self.loader, "_ZipLoader__check_param", return_value=True
        ), mock.patch.object(self.loader, "_ZipLoader__init_param"), mock.patch.object(
            self.loader, "_ZipLoader__get_dm_zip"
        ) as m1, mock.patch.object(
            self.loader, "_ZipLoader__get_response"
        ) as m2:
            m1.return_value = [{"k1": "v1"}]
            m2.return_value = [{"k2": "v2"}]
            body = self.loader.get_body({"zip_info": {"zip": 123}})
            self.assertEqual(body["result"], [{"k2": "v2"}])

※長期的に改修されているソースコードのためコーディングルールが整っていないコードになります

上記を見て頂くと分かる通り、プライベートメソッドに対してモックが当てられ、また、プライベートメソッドそれ自体に対してもテストが書かれていたので、クラス内部の構造、具体的にはプライベートメソッドの引数や命名や呼び出し関係などを変えるリファクタへの耐性が低いユニットテストになっていたのです。こうなってしまった背景としてはZipLoderクラスが「入力値のバリデーション」、「DBからのデータ取得」、「レスポンスの整形処理」を担い、担当する責務範囲が巨大であったこと、そして、そのようなクラスに対してモックを多用することにより無理矢理ユニットテストをカバレッジ100%で書いていたことが原因と考えられます。クラスの責務分割とユニットテストのあるべき姿についてもまたひとつ大きなテーマだと思いますがここでは置いておき、このままではリファクタの前後のふるまいの担保が今の状態のユニットテストではできないことが分かりました。

ここで特筆すべきは、ユニットテストが書かれてあっても、その書かれようによってはリファクタリングに活用できない場合もあるということです。リファクタリングを進める際には、プライベートメソッドの構造、入出力、担わせる責務を変える場合が多いです。しかし、プライベートメソッドがモックされていたり、プライベートメソッドそのもののテストが書かれていると、これらの変更でほぼ確実にテストが失敗するようになります。ふるまいの担保のためにテストを使うには、コード変更の前後でテストを修正せずともテストがパスすることが必要なためこれでは少なくともふるまいの担保の目的では使えません。（もちろんそのプライベートメソッドの仕様を把握するという目的では役には立ちます。）

そこで我々は方針転換し、インテグレーションテストのテストケースを拡充しつつリファクタを進めることにしました。リファクタリングを加えようとする部分を通るようなテストケースを書き、そのテストがパスするのを確認しながらリファクタリングを進めれば、ある程度のふるまいの担保は保証できるはずです。ただし、このインテグレーションテストについても、リファクタリングにそのまま使えるかというとそうではありませんでした。この点について次節に記述します。

インテグレーションテストの気軽な実行・変更ができない

ここまでの活動で、既存のインテグレーションテストにテストケースを追加して、ふるまいの担保をそちらに任せつつリファクタリングを進める方針に決まりました。そこで、既存のインテグレーションテストについて調べたところ、既存のインテグレーションテストはリモートに置かれたテスト用APIサーバーに対してリクエストを投げ、そのレスポンスが期待値と一致するかを確認するものになっていました。そのため、変更したコードに対してのインテグレーションテストの実行のためには、リモートのテスト用サーバーへ変更したコードをデプロイする必要がありました。またテストの実行もJenkinsジョブとして用意されてあり、一部のテストケースに限定した実行ができない状態であり、全テストケースの実行には6～7分かかる状態でした。効率的にリファクタリングを進めるためにはコードの変更とテスト実行のイテレーションを高速に繰り返せるようにすることが必要です。リファクタの度にリモート環境にデプロイし、6～7分かかるテスト実行が完了するのを待つのではリファクタリングを効率的に進められません。そこで我々はまずリファクタリングを進めるために、テスト実行を素早くできる環境を構築することに力を注ぎました。具体的に実施したことは以下です。

下記を手元のローカル環境で立ち上げられるようにDockerコンテナ化
1. APIサーバー
2. DBサーバー
3. インテグレーションテストサーバー（APIサーバーに対してテストリクエストを投げるサーバー）
インテグレーションテストのテストケースを選択して実行するコマンドの調査

1により、ローカル環境でコマンドを2つ実行するだけでコンテナの起動とインテグレーションテストの実行ができる環境を構築できました。また、2により、テストケースを選択して実行できるようになったので、6～7分かかっていたテスト実行が、リファクタリングに関係のないエンドポイントに対するテストケースを実行しないことにより、数十秒で完了できるようになりました。

また、別の問題として、テスト用APIサーバーが参照するテスト用DBへのデータ投入はDB初期化時に一括して行われ、一部のテストケース中でもレコードの追加・削除が行われる状態であり、これが原因で、すべてのテストケースが期待された順番で実行されないとインテグレーションテストが失敗する可能性がある状態であることが分かりました。

そこで我々は、テスト用DBへのデータ投入タイミングを再考しました。初期化時に一括でデータを投入し、途中のテストケースで一部のデータに変更が入る構成だと、テストケース間に依存関係ができてしまい、新たなテストケースの追加やテストデータの追加のハードルが大きいです。目的にもよりますが、リファクタのためにテストケースとそのためのデータを追加するにはこの構成だと扱いづらいので、初期化時にはデータベースとテーブルの作成のみ行い、各テストケース内で、そのテストケースが必要とするデータの投入、終了時に追加したデータを削除する構成にしました。この構成で書かれたテストケースの一例が以下のコードです。これによって変更を加えたいエンドポイントのインテグレーションテストへのテストケースの追加を他のテストケースとの依存関係を考えずに行うことができるようになりました。

class ZipTest(unittest.TestCase):
    def teardown(self):
        # 追加したデータを削除
        self.clear_dm_zip()

    def test_valid_zip(self):
        """正常系：zip=1000000"""
        # テストに必要なデータの追加
        self.insert_dm_zip(1000000, "町名", "チョウメイ", "町名補足", "住所0")

        search_query = {"zip": 1000000}
        response = self.post(search_query)
        j = self.assertValidApiResponse(response, validate_schema=True)
        self.assertEqual(j["result"][0]["zip"], 1000000)

結論

上記で紹介した活動により、ようやく満足にリファクタリングを進められる準備・環境を整えられました。

今回学んだことは、リファクタリングをいざ始めようとした場合に、リファクタリングそれ自体ではなく、その準備にも困難が立ちはだかるということです。そもそも業務ではリファクタリングに無限の時間を投下はできず、費やせる時間の範囲内で実行できるリファクタリング内容を選定することが求められます。また、コードをリファクタリングする場合はふるまいを変えていないことを担保するための何らかの方法を用意することが求められます。既存のテストがリファクタリングの役に立たない場合もあります。実際にリファクタリングを進めていく際には「コードの変更→テストの実行」のサイクルを高速に実行できる環境がないと効率的にリファクタリングを進められないため、この環境の構築が必要な場合があります。リファクタリングのためには、これらの困難を一つ一つ乗り越えていく必要があり、その克服方法にもある程度ノウハウが求められます。今回我々が遭遇した困難とその解決方法が皆様がリファクタされる際の参考になれば幸いです。

モノタロウでは随時エンジニアの採用募集をしております。この記事に興味を持っていただけた方や、モノタロウのエンジニアと話してみたい！という方はカジュアル面談登録フォームからご応募お待ちしております。