DLL読み込み失敗のテストを追加する #1819
DLL読み込み失敗のテストを追加する #1819
Conversation
|
✅ Build sakura 1.0.4087 completed (commit 4bd2afbde3 by @berryzplus) |
|
✅ Build sakura 1.0.4088 completed (commit a1a6c11e9a by @berryzplus) |
比較的簡単に対処できる不都合を解消する ・ヘッダーの参照不足で、利用側コードにincludeを書かないといけない ・クラス宣言が継承不可になってるためにテストを書けない
berryzplus
force-pushed
the
feature/add_test_of_extmodules
branch
from
f4d1452 to
3b06a42
Compare
|
✅ Build sakura 1.0.4089 completed (commit 8f2b06f785 by @berryzplus) |
プロセス内に1つのみ存在するシングルインスタンスにする
|
❌ Build sakura 1.0.4090 failed (commit d3c636f243 by @berryzplus) |
|
❌ Build sakura 1.0.4091 failed (commit d3c636f243 by @berryzplus) |
|
❌ Build sakura 1.0.4092 failed (commit e1d704baf0 by @berryzplus) |
This reverts commit 22b2a51.
|
基底クラスのコンストラクタがnoexceptでないのにCMigemoのコンストラクタをnoexceptと定義していたのが、Appveyorビルドだけエラーになっていました。 何故問題が発生したのかは不明。 |
|
✅ Build sakura 1.0.4093 completed (commit d844fa4630 by @berryzplus) |
| @@ -31,7 +31,7 @@ typedef struct uchardet * uchardet_t; | |||
| /*! | |||
| * uchardet ライブラリ(uchardet.dll) をラップするクラス | |||
| */ | |||
| class CUchardet final : public CDllImp | |||
| class CUchardet : public CDllImp | |||
There was a problem hiding this comment.
単体テストをしたいから本来は付けた方が良いキーワードを外すというのはあんまり良くない気がします。
There was a problem hiding this comment.
やろうとしてることは、
「実装の正当性を担保したいから」
「検証にあたって不都合なキーワードを外す」
です。
他の方法で「実装の正当性」を担保できているなら要らないです。
「本来は付けた方が良い」に関しては、
何故「付けた方が良い」になるか理解できていません。
finalキーワードを付けるとクラスが継承不可になるので、
派生クラスに対するインターっフェースを定義する必要がなくなり、
クラス設計の大部分を省略できます。
（≒めんどくさい作業を省略できる）
「検証できないコードは不具合である」と仮置きして作業していますが、
finalキーワードを付けるメリットは、検証できないデメリットを補えるものなのでしょうか？
継承を使わずにコードの検証ができる方法があれば、そっちに切り替えてもよいです。
手作業の検証だと人為的ミスの余地があるので自動テストが良いです。
There was a problem hiding this comment.
出来れば非侵襲・非破壊で検査したいですがそうもいかないですよね。
FINALとかいうマクロは出来るだけ使いたくないので、継承しないでテスト出来る方法が無いか考えてみます。
There was a problem hiding this comment.
ぱっと見ですが tests/unittests/TExtModule.hpp ファイルのクラスを使わないでテストコードを書けばいいんじゃないかと思いました。
There was a problem hiding this comment.
ぱっと見ですが tests/unittests/TExtModule.hpp ファイルのクラスを使わないでテストコードを書けばいいんじゃないかと思いました。
TExtModule.hppを使わなければ、final を外さなくてもテストが書けるのは分かっています。
tests1.exeのカレントフォルダに "uchardet.dll" という名前のDLLを用意すれば、
コード記述量は多いですが「不可能ではない」と思います。
ステップ数も少ないですし、そこまでしてこのクラスをテスト可能にしたいとは思わないです。
自分は finalを外すデメリットは「ない」と思っています。
もしかして、そこに認識齟齬があるのかしら？
There was a problem hiding this comment.
もうちょっとコードを読んでみました。
継承して GetDllNameImp メソッドがoverrideして返す値を置き換える事で試験しているように見えます。
FailedToLoadLibrary はLoadLibraryが失敗するケースでのテストだと思いますが、パスが通っている場所にDLLが存在するとそれを読んで成功してしまうので、必ず失敗するように存在しないファイル名に置き換えているっぽいです。
FailedToGetProcAddress は本来使うのとは違うDLLを指定する事でGetProcAddressが失敗する確認を行っているように見えます。
手法的にはお手軽にテストする為に本体側のコードを変更しているのが強引に感じるところです。まぁテストの為に private や protected を public にしたりもよくある話ですが…。
final を外すデメリットは現状無いのかもしれませんが、何らかの理由で継承をさせない為に付けたくなった場合にテストコードが継承を使っているから付けられない、という事態になりそうです。
まぁ継承を禁止しないと実装出来ないのか？というと必ずしもそうではないと思います。final はまぁ今のところ継承するつもりないしとりあえず付けとくか、みたいな感じで自分は使ってます。まぁ軽い意図なので変更しても問題は無いけど強引だなぁと感じるくらいです。
finalキーワードを付けるとクラスが継承不可になるので、
派生クラスに対するインターっフェースを定義する必要がなくなり、
クラス設計の大部分を省略できます。
（≒めんどくさい作業を省略できる）
これは場合によって使い方というか逼迫度が異なると思います。
自分が付けた理由は、berryzplusさんのコメントにあるような、派生される事で想定しない挙動が生じうるのを防ぐ為にとりあえず指定、です。
ただ本当に真剣に継承されてしまうと問題が起きる（もしくは起きやすい）ケースで継承をさせたくないので final を付けるというケースはあるかもしれません。仮想関数によるpolymorphismとか使わないようなケースですね。あとは仮想関数の override を防ぐ為に final 指定する事も出来るので、その場合は override してほしくない強い意図を感じるのでテストしたいからといって外すのは良くない気がします。仮に外すならテスト用のビルドだけとかにした方がいいかなと思います。
There was a problem hiding this comment.
いまのところ、基底クラスも仮想メンバも持たないクラスや、パッと見では「十分に具象化された概念を表している」とは判断できないクラスに付けられたfinalを外すつもりはないです。
- 基底クラスも仮想メンバも持たないクラスは
finalを外してもメソッドのオーバーライドができないので外す意味がないです。「将来的にも継承させるつもりはない」の意思表示は「尊重すべき」と思います。 - パッと見では「十分に具象化された概念を表している」と判断できないクラスに付けられた
finalは必要だと思います。単に「名前が悪い」という話ではなく、そういうケースはあると思います。この場合に「派生させたくない」という意思表示はfinalでやるしかないと思います。「何故派生させたくないのか？」が少し気になりますが、あり得る話です。
CUchardet.hのfinalを外した場合はどうか。
このクラスはパッと見で「十分に具象化された概念を表している」と判断できます。
finalを外すと「俺が作ったカスタムUchardet.dllを扱える専用クラス」という謎の派生品を作れるようになるのはデメリットですが、たぶん誰もやらんんので問題ないと思います。
There was a problem hiding this comment.
「十分に具象化された概念」というやや難しそうな言葉が登場しましたね。考えを整理する為に実装とは別の捉え方を導入すると解釈の違いで意思疎通が難しくなりそう…。
final を付けるべきか付けないべきかというのはプログラマ任せになりますが、判断基準は単に継承を許可したいかそれともしたくないかという事です。で、継承を許可したくない理由として継承に対応する事を考慮して実装をしていないから、で特に問題が無い気がします。それに対して何故というのを突き詰めて考えるのはテストコードを書く際にやる事ではない気がしますね。
結局のところberryzplusさんが実際にやっているのは、本来継承を想定していないクラスに対して、テスト方法として継承を使うようにする為に final を外している、です。継承せずにそれ以外の方法でも（手間は掛かりますが）行いたいテストは出来るわけなので、あんまり良くない、という感想に繋がります。
継承しない場合のテスト方法ですが、
- ファイル名は合っていてLoadLibraryが出来るけれどシンボルが無くてGetProcAddressに失敗するケース
- そういうDLLファイルを作成すれば試験出来ます。しかし作成するのが手間なのでテストコードで継承を使っているように見えます。
- ファイルが見つからなくてLoadLibraryが出来ないケース
- 元々の実装で読み込むファイル名は決めた実装になっているのに、そこを無理やり変えてファイルが見つからない挙動のテストをしているのはちょっと強引です。
- PATHが通っている場所に試験対象のDLLを置かないでテストコードを動かす必要がありますが、もしかしたらDLLによっては簡単にはいかないかもしれません。
- DLLのファイルの内容が不正でLoadLibraryが出来ないケース
- 今回思いつきましたが、こういう場合にLoadLibraryは失敗するはずです。
継承禁止(final)を外すのを良くないと言われて「いやこのケースでは別に外すのは問題が無いんだ」というのがberryzplusさんの答えになっていると思いますが、本当に外すのが問題があるケースでは結局継承を使わずにテストする必要がありますよね？「その場合は継承使わないでテストするから問題ないよ」と言うのかもしれませんが…。
There was a problem hiding this comment.
継承を使わずにテストできる方法を思いつかないので、finalを外させてほしいです。
- FailedToLoadLibrary 現実的に代替方法が存在しません。
- FailedToGetProcAddresse DLL検索パスにカスタムのUchardet.DLLを置けば代替可能。
派生クラスの実装では上記2つを区別しなくてよいので、FailedToGetProcAddresseに相当するテストケースを用意すれば「finalを外さずにテストを実現する」ことは可能です。CUchardetの実装はこうなってるのでFailedToLoadLibraryをテストする必要はないかも知れません。
とりあえず、これが通らないと先に進めないことが問題だと思っています。
逆に、これが通ると #1815 で検証していたスタブDLLの仕組みを使って「外部DLLを読み込むクラス」に対する「スタブDLLを使ったテスト」を導入する流れにしていくつもりです。
There was a problem hiding this comment.
変更自体は許容出来るのでApproveしました。必要に応じてまた後から変更は出来ますし。
「継承を使わずにテストする方法は思いつかない」と書かれてますが、そのすぐ後に少し案が書かれているような気がします。
FailedToGetProcAddresse については「DLL検索パスにカスタムのUchardet.DLLを置けば代替可能」と書かれていますがこれはGetProcAddressで読むシンボルが無いダミーのDLLファイルをDLL検索パスに置けばテスト出来るだろうという事ですよね？自分もその方法は思いつきました。そのDLLを作る記述を用意すれば出来ると思いますがちょっとその用意が億劫ですね。
FailedToLoadLibrary は仮にDLL検索パスに探すDLLファイルが存在してしまっていると、LoadLibraryでDLLが見つからないという挙動が再現出来ないという事ですかね？
自分では色々と試してないですがそういう事かと想像しています。
WindowsAPIの SetDllDirectory に空文字を指定する事で、DLL検索パスからカレントディレクトリを取り除くことが出来るようです。そして下記の順にDLLを探すようです。
- アプリケーションが読み込まれたディレクトリ
- システム ディレクトリ
- 16 ビット システム ディレクトリ
- Windows ディレクトリ
- PATH環境変数のディレクトリ
- SetEnvironmentVariableでPATHを変更すればそのプロセスの分は制御出来そう
という事で問題になりそうなのは、CHtmlHelp のテストでしょうか？ CHtmlHelp::GetDllNameImp で HHCTRL.OCX を返していますが自分の環境でコマンドプロンプトで where HHCTRL.OCX とすると C:\Windows\System32\hhctrl.ocx と表示されたのでシステムディレクトリに含まれているように見えます。
これだとファイルが存在しないというのが試験出来ないですね。なので代替案としては同名の読み込めないファイルをカレントディレクトリに置く事でしょうか…。
berryzplusさんがこのPRで行っている読み込むファイル名を強引に書き換えてるというのは元の実装の挙動ではないですが、読み込みに失敗した場合にどう振る舞うかを確認する試験といった感じですかね。まぁ元のまんまだと試験が出来ないのでどこにスタブを用意するかの話になるんですかね。
参考資料
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setdlldirectoryw
|
Kudos, SonarCloud Quality Gate passed! |
|
✅ Build sakura 1.0.4095 completed (commit 398d83d45f by @berryzplus) |
|
レビューありがとうございます。マージしてしまいます。 |
PR の目的
タイトル通りです。
カテゴリ
PR の背景
#1722 で報告された
CPPA::stdErrorの問題（？）に対処するために自動テストを整備する過程で、用意したテストコードを複数クラスで使いまわせそうなものがあることに気付きました。
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
外部DLLを読み込むクラスに共通する、読込失敗時のテストを追加します。
テスト仕様
::LoadLibraryWの呼出に失敗した場合の挙動をチェックします。CDllImpl派生クラスで定義した名前のDLLが見つからない場合のテストです。
DLLが見つからないときはIsAvailable()がfalseを返します。
::GetProcAddressの呼出に失敗した場合の挙動をチェックします。CDllImpl派生クラスで定義した名前のエクスポート関数が見つからない場合のテストです。
定義されたエクスポート関数のうち、1つでも見つからないときはIsAvailable()がfalseを返します。
当初テスト導入のために本体修正が必要なクラスはテスト対象から外そうとしましたが、本体改修を避けると追加でカバーできる範囲が不当に狭くなることが分かったので、本体改修も微妙に含めるようにしました。
今回同時に改修する本体側の不都合
CHtmlHelp.hインクルードファイルの記載が漏れていたため、利用側でインクルードする必要がありました。CIcu4cI18n.hクラス定義にfinalが付いていたため、継承を前提とするテストを書けませんでした。CUchardet.hクラス定義にfinalが付いていたため、継承を前提とするテストを書けませんでした。テスト対象としないクラスがいくつかあります。
対応しない理由
CMigemoシングルトンとして定義されているため。CUxThemeシングルトンとして定義されているため。CDllPluginCDllImpのDLL読み込み機能を利用していないため。追記：
CMigemoは「エディタプロセス内に1つ存在」の仕様が明らかなので、シングルトンを解除して対応することにしました。CUxThemeとCDllPluginについては、動的なロード＆アンロードをサポートするためのクラスCDllImplを継承すること自体が「誤り」と考えられるため、対応せずに放置します。※
CUxThemeはアンロードを想定していません。※
CDllPluginは動的ロード＆アンロードを想定していますが、CDllImplの仕組みを無視して独自にロード処理を行っています。ロード処理を健全化することは可能ですが、現状でテストに対応させる意味はありません。PR の影響範囲
テスト件数が増えます。
本体改修の内容は「コメントを修正したようなもの」なので機能への影響はないと考えられます。
テスト内容
このPRの目的はテストを追加することです。
関連 issue, PR
参考資料