Mobile Factory Tech Blog

技術好きな方へ!モバイルファクトリーのエンジニアたちが楽しい技術話をお届けします!

Perl::Critic の Policy を作ってチーム独自のコーディング規約チェックをする

概要

こんにちは、エンジニアの id:mp0liiu です。
自分は Perl でチーム開発をしているのですが、最近ある外部モジュールの使い方に関するチーム独自のコーディング規約が追加されました。
このコーディング規約に沿ってコードが書けているかどうかは人間の目でチェックする必要があるので、手間がかかりますし、開発時やコードレビュー時に見落としてしまう可能性があります。
そこで Perl コードの構文チェックをするツール Perl::Critic の Policy(ルールのような概念)を作り、チーム独自のコーディング規約を機械的にチェックできるようにしました。
この記事ではその際したことをまとめ、チーム独自のコーディング規約のチェックのために Perl::Critic の Policy が作れるよう、 Perl::Critic の Policy の作り方を解説します。

今回つくった Policy の完全版はこちらのレポジトリにあります。

経緯

自分が担当しているプロダクトでは Data::Validator を利用して引数の型チェックを行っています。

Data::Validator では 通常 validate メソッドで渡した値を名前付き引数としてチェックします。

use Data::Validator;

sub add {
    # 引数の型の指定
    state $v = Data::Validator->new(
        x => 'Int',
        y => 'Int',
    );
    # 引数の型をチェック
    my $args = $v->validate(@_);
    return $args->{x} + $args->{y};
}

add(x => 2, y => 3);

しかし、引数が1つなのに名前付き引数を渡すのは大抵の場合冗長です。
なので最近チームで引数が1つの場合は Data::Validator の拡張機能 StrictSequenced を使って普通の引数として渡すようにする、というコーディング規約が追加されました。

use Data::Validator;

sub named {
    state $v = Data::Validator->new(num => 'Int');
    my $args = $v->validate(@_);
    return $args->{num};
}

# 通常はこのように引数が1つでも名前付き引数として渡す必要がある
named(num => 1);

sub sequenced {
    state $v = Data::Validator
        ->new(num => 'Int')
        # Data::Validator の拡張機能 StrictSequenced を使う
        ->with('StrictSequenced');
    my $args = $v->validate(@_);
    return $args->{num};
}

# StrictSequenced を使っている場合は引数だけを渡す
sequenced(1);

この規約が導入されると、コードレビューなどの際にコードの書き方をチェックする必要がでてくるわけですが、コードの書き方のチェックを人間がやるのは大変です。
なので構文チェックツールなどで機械的に書き方のチェックをしたくなり、 Perl::Critic の Policy を作ることにしました。

前提知識

Perl::Critic の Policy を実装するにあたって、次のドキュメントを読みました。

  • PPI のドキュメント
  • Perl::Critic::Util のドキュメント
  • Perl::Critic::Policy のドキュメント

Perl::Critic では PPI で静的解析をして構文チェックを行っているため、自分で Policy を実装する場合も PPI のドキュメント構造を走査するようなコードを書くことになります。 なので、PPIの使い方や PDOM(Perl Document Object Model)について理解しているとスムーズに実装が行えるでしょう。

Perl::Critic::Util では Policy を実装するにあたって役に立つような PDOM をパースする関数が提供されています。 簡潔に処理を書けたり、自前で実装すると大変な処理が利用できることもありますので、どんな関数があるか目を通しておくといいと思います。

Perl::Critic::Policy は、全ての Policy の親クラスとなるクラスで、 Policy の実装やAPIについて記述されています。

これらのドキュメントに目を通した上で、既に実装されている Policy のコードを読み、実装の参考にしました。
今回、自分は Perl::Critic::Policy::TryTiny::RequireUsePerl::Critic::Policy::Subroutines::ProhibitManyArgs を参考にしました。

コードと解説

以上のようなドキュメントに目を通した上で Policy を実装したので、どのようなコードを書いたのかを解説します。

package Perl::Critic::Policy::DataValidator::RequireStrictSequenced;
use strict;
use warnings;
use utf8;

our $VERSION = '0.01';

use List::Util qw( any );
use Perl::Critic::Utils qw(
    $SEVERITY_LOWEST
    parse_arg_list
    is_class_name
    is_method_call
);

# (1) Perl::Critic::Policy を継承して実装
use parent 'Perl::Critic::Policy';

# (2) サポートする Policy の設定値
sub supported_parameters { return () }

# (3) デフォルトのチェックの厳しさ
sub default_severity { return $SEVERITY_LOWEST }

# (4) デフォルトのtheme
sub default_themes { return qw( cosmetic ) }

# (5) この Policy が対象とする PPIのクラス名
sub applies_to { return 'PPI::Statement::Variable' }

# (6) Policy の構文チェック処理本体
sub violates {
    my ($self, $stmt, $doc) = @_;

    # (7) 'Data::Validator->' とマッチするか
    my $data_validator = $stmt->find_first(sub {
        my (undef, $child) = @_;
        return is_class_name($child) && $child->content eq 'Data::Validator';
    });
    return unless !!$data_validator;

    # (8) '->new' とマッチするか
    my $guess_new = $data_validator->snext_sibling->snext_sibling;
    return unless is_method_call($guess_new) && $guess_new->content eq 'new';

    # (9) Data::Validator->new には名前付き引数としてハッシュでパラメータをが渡されているはず
    my @args_passed_to_new = parse_arg_list($guess_new);
    return if @args_passed_to_new % 2 == 1;

    # (10) 引数がない場合はチェックしない
    my $params_num = int @args_passed_to_new / 2;
    return if $params_num == 0;

    # (11) new の後で '->' とマッチするか
    my $guess_method_call_operator = $guess_new->snext_sibling->snext_sibling;
    return unless defined $guess_method_call_operator;

    # (12) '->with' とマッチするか
    my $guess_with = $guess_method_call_operator->snext_sibling;
    return unless is_method_call($guess_with) && $guess_with->content eq 'with';

    # (13) with で渡されている引数を見て StrictSequenced が使われているか調べる
    my @args_passed_to_with  = parse_arg_list($guess_with);
    my @extensions_name      = map { $_->literal } map { @$_ } @args_passed_to_with;
    my $has_strict_sequenced = any { $_ eq 'StrictSequenced' } @extensions_name;

    # (14) 引数が1つでかつ StrictSequenced が使われていなければ Policy 違反を報告
    if ( $params_num == 1 && !$has_strict_sequenced ) {
        return $self->violation(
            q{You should use 'StrictSequenced', an extensions of Data::Validator.},
            q{Passing named arguments when a subroutine has only one argument is redundant.},
            $stmt
        );
    }
    return;
}

1;

Policy を実装するには、まず (1) のように Perl::Critic::Policy を継承します。

(2) の supported_parameters メソッドは、 Policy でサポートする設定値のリストを返すメソッドです。今回は特に設定値使うことを考えていなかったので空のリストを返しています。

(3) の default_severity メソッドは、 Policy デフォルトの厳しさ1を返すメソッドです。

(4) の default_themes メソッドは Policy がデフォルトで属する Perl::Critic のテーマ2のリストを返すメソッドです。適切な設定かどうかの自信はあまりないのですが、今回はコードの健全さに影響するテーマとして maintenance を指定しています。(独自の theme を設定することもできるのかもしれませんが、未検証です。)

(5) の applies_to メソッドはこの Policy が対象とするPPIのクラス名を返すメソッドです。
Policy が構文チェックをするときは、 構文チェック中のコードからこのメソッドが返したクラスの PDOM を全部取得して、 取得したPDOMごとに (6) のチェック処理本体を呼び出すという挙動をします。
何もオーバーライドしない場合のデフォルトは PPI::Element なので、すべてのPDOMを対象に Policy のチェック処理が走ることになりますが、
より詳細な対象クラスを指定するとチェック対象のPDOMが絞り込まれるのでパフォーマンスが向上します。
今回は Data::Validator で引数の型チェックをする場合一度変数に Data::Validator のインスタンスを格納することが多いかと思い、 変数宣言文のクラスである PPI::Statement::Variable を指定しています。

(6) の violates メソッドに Policy のチェック処理本体を記述します。 第1引数には applies_to で指定されたチェック対象のPPIクラス(今回は変数宣言文のクラスである PPI::Statement::Variable )のPDOMが、第2引数にはチェック中のコード全体のPPIドキュメントが渡されます。

(7) では変数宣言文のPDOM $stmt の子要素に Data::Validator-> という文字列にマッチするトークンがあるかどうかを、find_first という引数に渡したコールバックが最初に真を返した要素を取得するメソッドで調べています。
子トークンが Data::Validator-> とマッチしているかは、クラス名であるかどうかを判定する is_class_name 関数と子トークンの中身が Data::Validator であるかを調べて判定しています。

(8) では Data::Validator とマッチした部分のトークンの隣でコンストラクタが呼ばれているかどうかを調べています。
まずコンストラクタとしてのメソッド名 new があると期待される Data::Validator の次の次のトークンを、 snext_sibling という動作に影響しない空白のようなトークンを除いた、次のトークンを参照するメソッドで取得しています。
そしてそれがメソッド呼び出しかどうかを is_method_call 関数で判定し、トークンの中身が new であるかを調ることで、 Data::Validator からコンストラクタが呼ばれるコードであるかを調べています。

(9)、(10) では、 parse_arg_list 関数でコンストラクタにどのような引数が渡されたかを解析し、 Data::Validator のコンストラクタに渡されている引数の数、すなわちそのメソッドの仮引数の数を調べています。

(11)、(12) では Data::Validator の拡張機能を使う with メソッドが呼ばれているかを調べ、
(13) で with メソッドに渡されている引数を解析し、 StrictSequenced が使われているかを調べています。

そして (14) でいよいよ仮引数の数が1つの場合 StrictSequenced が使われているかどうかを調べ、使っていなければ violation メソッドで Perl::Critic::Violation のインスタンスを作って Policy の違反を報告しています。

Policy のテスト

テストがあった方が開発もメンテナンスもしやすいので、 Policy のテストを書きながら開発を進めたいところです。
Perl::Critic のインスタンス生成時、引数 --single-policy にテストしたい Policy を指定しその Policy だけで構文チェックをさせるようにした上で、
構文チェックするメソッド critcue にスカラリファレンスで文字列を渡しその文字列をコードとして扱わせ構文チェックすると、テストがしやすいのでおすすめです。

use strict;
use warnings;
use utf8;
use Test2::V0;
use Perl::Critic;

my $critic = Perl::Critic->new(
    '-single-policy' => 'DataValidator::RequireStrictSequenced',
);

my @violations = $critic->critique(\q{
    sub one_args {
        my $v = Data::Validator->new(
            "num" => 'Int',
        )->with(qw/ Method StrictSequenced /);
    }
});
is @violations, 0;

done_testing;

実装した Policy を使う

実装した Policy のモジュールが Perl::Critic::Policy 名前空間に属していて、かつ Perl::Critic を動かしている perl から実装した Policy がロードできる状態になっていれば、 Perl::Critic は Policy のモジュールを自動的にロードしてくれます。
なので、後は設定ファイル(通常は .perlcriticrc)で次のように Policy名(Policyのパッケージ名から Perl::Critic::Policy:: を除いたもの ) を追加する設定を記述するなどすれば実装したPolicyが有効になります。

include = DataValidator::RequireStrictSequenced

設定を追加したらCIを回す時にコーディング規約にそったコードになっているかテストしたり、エディタでチェックしたりと活用することができるようになります。

おわりに

このように、 PPI や Perl::Critic についての知識があれば、チーム独自のコーディング規約をチェックする Policy を実装できます。
Perlのプロジェクトで人間の目でチーム独自のコーディング規約をチェックしていて、チェックに要する手間や見落としが問題になっているのであれば、 Perl::Critic の Policy を実装して機械的にチェックさせてみてはいかがでしょうか。


  1. どれくらい重大な違反なのかを示すパラメータです。詳細はこちら

  2. カテゴリのような概念です。詳細はこちら

巨大なリポジトリのJenkinsからCircleCIへの移行においてshallow cloneとsparse checkoutで前処理を高速化する

はじめに

こんにちは!モバイルファクトリー Advent Calendar 2019 24日目担当の@PikkamanVです。 今回は運用中のプロダクトのCIをJenkinsからCircleCIへ移行するにあたり特にハードルが高かった点の解決方法を紹介します。 オンプレのJenkinsサーバでフルテストを回すのが前提となっていたリポジトリをCircleCIで扱うにあたり、shallow cloneとsparse checkoutを活用することでテストの前準備の高速化を図りました。

背景

今回扱うリポジトリは物理サーバ上に開発環境を用意することが前提になっており、テストも同様に単一の物理サーバ上で実行されていました。長期間の運用により総コミット数は90,000回を超え、リポジトリのサイズは30 GB弱となっていましたが、社内の強力なサーバ上でJenkinsを利用することで高速にテストを実行できていました。サーバ上に常にローカルリポジトリが存在するので、最新のコミットとの差分だけをリモートリポジトリから取得すればよかったためです。

しかし、CircleCIは通常の設定のままcheckoutするとまっさらな状態からリポジトリをコピーするので、歴史ある巨大なリポジトリではテストの前処理にサイズに比例して時間がかかる問題がありました。そこで、CircleCIにおいてgitのshallow cloneとsparse checkoutを活用することで高速にリポジトリをコピーし、テストの実行開始を早めることを目指しました。

shallow clone

今回のリポジトリは約5年間の運用を経て .git のサイズは約18GBにまで大きくなっていました。リポジトリ全体のサイズが約30GBなので、その占める割合が分かります。そのため、通常のgit clone を行うと大変時間がかかりました。しかし、想定しているCI環境においては対象のブランチの最新コミットに対してのみテストが実行できれば十分です。そこで、shallow cloneを使うことよりcheckoutの時間を短縮しました。

command: |
      git init
      git remote add origin "$CIRCLE_REPOSITORY_URL"
      git fetch --depth=2 origin "$CIRCLE_BRANCH"
      git fetch --depth=1 origin HEAD:refs/remotes/origin/HEAD
      git checkout "$CIRCLE_BRANCH"
      git reset --hard "$CIRCLE_SHA1"

sparse checkout

また歴史的経緯からリポジトリには画像などのサイズの大きいアセットが含まれており、約8.5GBを占めていました。これらをリモートリポジトリからコピーするには時間がかかります。よってsparse checkoutによって、サイズの大きいアセットはclone時に除外することを考えました。sparse checkoutは以下のようにcheckoutするディレクトリを .git/info/sparse-checkout に指定することで設定可能です。あるディレクトリAに含まれる特定のディレクトリBだけを除きたい場合は、ディレクトリA!ディレクトリA/ディレクトリB を両方指定することで実現できます。

command: |
      git config core.sparsecheckout true
      echo 'ディレクトリ1
      ディレクトリ2
      ...
      !除外するディレクトリ1
      !除外するディレクトリ2
      ...
      ' >> .git/info/sparse-checkout

しかし、除外したアセットに対してもそのファイルの存在を確認するテストなどが書かれていました。ですがサイズの大きなアセットこそが前処理の高速化においてボトルネックになっていたため、sparse checkoutはぜひ導入したい機能でした。ここでテストの中身に注目すると、テストされていた機能はめったに変更が入らず、また他の機能への影響も少ないものであることが分かりました。そこで、日中の開発中は対象のテストを除外して高速化を行う一方、1日1回すべてのテストを実行し、テスト実行時間と網羅性のバランスを取ることにしました。 まずは通常トリガされる build workflowの他に、1日1回実行される nightly workflowを用意します。nightly workflowでは、sparse checkoutによるアセットの除外を行わない checkout_code_full を設定しています。(上の例で !除外するディレクトリ1 などを削除するイメージです)

workflows:
  version: 2
  build:
    jobs:
      - checkout_code
      - frontend:
          requires:
            - checkout_code
      - backend:
          requires:
            - checkout_code
  nightly:
    triggers:
      - schedule:
          # UTC表記 平日の午前7時20分から実行
          cron: "20 22 * * 0-4"
          filters:
            branches:
              only:
                - develop
                - /base-.*/
    jobs:
      - checkout_code_full
      - frontend:
          requires:
            - checkout_code_full
      - backend_full:
          requires:
            - checkout_code_full

さらにアセット関連のテストも実行する backend_full ジョブにおいて環境変数 BUILD_TIME を設定します。

  backend_full:
    docker:
      - image: ...
        environment:
          ...
          BUILD_TIME: nightly
          ...

そして対象のテストの始めに環境変数を見てテストをスキップするかどうかの処理を追加しました。

use Test::More;
...

if($ENV{BUILD_TIME} ne 'nightly') {
    plan skip_all => 'No images';
}

subtest 'subtest_name' => sub {
    ...
}

done_testing;

こうして1日1回だけ実行されるnightlyジョブでアセット関連のテストを行うことができるようになりました。

まとめ

以上のように、shallow cloneとsparse checkoutを組み合わせた上にCircleCIのworkflowを使い分けることで

  • 日中の開発中はサイズの大きいアセットを除いてテストすることで実行時間を短縮し、開発のストレスを減少させる
  • 1日1回すべてのアセットを含んだテストを実行することでテストの網羅性を確保し、重大な開発の手戻りを防止する

の両方を実現することができました。巨大なリポジトリをCircleCIへ移行するにあたってヒントになれば幸いです。