Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests1.exeにzipリソースを添付する #1670

Merged

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented May 16, 2021

PR の目的

タイトル通りです。
Grepテストなどで使うために、テストモジュールにzipリソースを添付します。

カテゴリ

  • その他の問題

PR の背景

Grepテスト導入(#1660)に必要な変更です。

修正内容がうんこなのが気に入らなくて放置していましたが、
どんなうんこ仕様でもないよりはマシだろう、と考えてPR作成します。

PR のメリット

  • Grep機能のような、外部のファイルデータに依存する機能の単体テストを書けるようになります。

PR のデメリット (トレードオフとかあれば)

  • リソース添付を実現するためにMSVCビルドの構成を変更していますが、変更内容がうんこです。
  • リソース添付を実現するためにMinGWビルドの構成を変更していますが、変更内容がうんこです。
  • 添付されたzipリソースを検証するためにCZipFileのテストを追加していますが、テスト内容がうんこです。

仕様・動作説明

サクラエディタの仕様・機能には影響を与えない変更です。

コミットを見れば分かる通り、変更は3つのテーマで行っています。

  • 【主目的】 c6ae3fd 0125ca9 テストモジュールにzipリソースを添付します。
    • 【依存】 テストでリソースを使えるよう出力フォルダを統一する。
    • 【依存の詳細】 1cc18ee 6dba3a6 MSVCのビルド構成を修正する。
    • 【依存の詳細】 d4dcfa6 11755f1 MinGWのビルド構成を修正する。

MSVCのビルド構成が「うんこ」であるとする理由

サクラエディタのビルドステップには、ソース生成が含まれます。
Funccode_enum.hとgithash.hを生成するステップがこれにあたります。
現状これらファイルの生成は ClCompile ターゲット の前に行っていますが、
MsBuildのビルドステップには「ソース生成」を行うターゲットが存在するので、
適切なビルド構成であるとは言えません。

労力に対する改善効果が薄いと予想されるため、
あえて「うんこ」な状態(≒ブリッと出してみた実装をそのまま使う)で残しておきます。

MinGWのビルド構成が「うんこ」であるとする理由

サクラエディタのビルドステップには、ソース生成が含まれます。
makeプロジェクトで環境依存の情報を取り込みたい場合、
通常は、configureという名前のスクリプトを用意して行います。
configureの中身はスクリプトで記述し、必要なソース生成はここで行います。
現状の構成はすべてをMakefile内に記述しており、適切なビルド構成であるとは言えません。

この件も、労力に対する改善効果が小さ過ぎると思うので、
あえて「うんこ」な状態(≒ブリッと出してみた実装をそのまま使う)で残しておきます。

CZipFileに対するテスト内容が「うんこ」であるとする理由

CZipFileはレガシーコードです。

レガシーコードとは、単体テストがないコードです。
英語表記 Legacy code が示す通り、時代遅れの(負の)コード遺産です。

単体テストを用意してやることで、
レガシーコードは古くからあるコード資産に「昇華」できます。

今回、CZipFileの一部メソッドに単体テストコードを用意しました。

これにより、該当部分がコード資産っぽくなります。
しかし、CZipFileというクラスは、本来CPluginManagerの専用サブクラスであり、開発者が意図した通りの手順で、開発者が意図した通りの引数を渡さなければ動作しないので、テストがあると言えるか微妙です。
※たとえばIsOkがfalseを返したあとにSetZipを呼ぶとクラッシュします。

この件の対策には、労力に見合う効果があると考えていますが、本題(≒既存コードに搭載されたzip展開機能が流用可能であることを示して、テストコードで利用したい)から外れていくので、あえて「うんこ」な状態(≒ブリッと出してみた実装をそのまま使う)で残しておきます。

PR の影響範囲

  • MinGWのビルド構成に手を加えているため、中間生成物と一部のビルド成果物の出力フォルダが変わります。
  • MSVCのビルド構成に手を加えているため、中間生成物と一部のビルド成果物の出力フォルダが変わります。
  • テストモジュールにzipリソースが追加されます。
  • CZipFileクラスのzip展開機能について、テストが追加されます。

テスト内容

アプリの実装を変えず、テスト追加とビルド構成の変更のみを行うPRなので、CIビルドのみで変更効果を確認できると思います。現在GitHub Actionsが障害中のようなのでビルドが完了するまでDraft状態にしておきます。

関連 issue, PR

#1660

参考資料

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

berryzplus commented May 16, 2021

Azure Pipelines MinGWビルドのyamlがまちがっていたので修正します。

GitHub Actionsの障害(=ActionsがQeue実行されない)は、直ったかもしれないです。

@AppVeyorBot
Copy link

Build sakura 1.0.3766 failed (commit 658d01a32c by @berryzplus)

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

MinGWビルドの失敗原因はgtest.dllがPATHの通ったディレクトリに存在しないため。
パッケージをやめてstaticリンクできないか考え中。
だいぶめんどくさいのでまたしばらく放置するかも。

@berryzplus berryzplus force-pushed the feature/add_resource_to_tests1 branch from 94cbe3d to 0125ca9 Compare May 18, 2021 00:22
@sonarcloud
Copy link

sonarcloud bot commented May 18, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

残課題

  • ビルド時zip圧縮スクリプトが雑。
  • CZipFile のテストケースの名前がいい加減。
  • 作成した CZipFile のテストでは、言語リソースを日米分ける必要なし(今後追加するテストのためだけに多言語化しているが、本質的には単なる無駄リソース)

テストコードなので、目的さえ達していればどうでも良いような気はします。

なお、今回新たに作り込む CZipFile テストの雑実装は、Grepテストを導入してから対処したいと考えています。
この件は設計レベルのリファクタリングをしないといかんので。

@berryzplus berryzplus marked this pull request as ready for review May 18, 2021 11:56
@berryzplus
Copy link
Contributor Author

とらあえずマージしちゃいます。

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants