-
Notifications
You must be signed in to change notification settings - Fork 167
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
DLL読み込み失敗のテストを追加する #1819
Merged
berryzplus
merged 7 commits into
sakura-editor:master
from
berryzplus:feature/add_test_of_extmodules
Mar 21, 2022
Merged
DLL読み込み失敗のテストを追加する #1819
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
62f60b4
DLL読み込み失敗のテストを追加
berryzplus 3b06a42
既存コードの不都合を解消する
berryzplus c8a993a
CMigemoをテスト可能にする
berryzplus 22b2a51
CDllImpのコンストラクタをnoexceptにする
berryzplus bb10431
Revert "CDllImpのコンストラクタをnoexceptにする"
berryzplus 586d0e2
CMigemoコンストラクタのnoexceptを外す
berryzplus cff81c1
Update tests/unittests/test-extmodules.cpp
berryzplus File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
/*! @file */ | ||
/* | ||
Copyright (C) 2022, Sakura Editor Organization | ||
This software is provided 'as-is', without any express or implied | ||
warranty. In no event will the authors be held liable for any damages | ||
arising from the use of this software. | ||
Permission is granted to anyone to use this software for any purpose, | ||
including commercial applications, and to alter it and redistribute it | ||
freely, subject to the following restrictions: | ||
1. The origin of this software must not be misrepresented; | ||
you must not claim that you wrote the original software. | ||
If you use this software in a product, an acknowledgment | ||
in the product documentation would be appreciated but is | ||
not required. | ||
2. Altered source versions must be plainly marked as such, | ||
and must not be misrepresented as being the original software. | ||
3. This notice may not be removed or altered from any source | ||
distribution. | ||
*/ | ||
#include <gtest/gtest.h> | ||
|
||
#include "extmodule/CDllHandler.h" | ||
|
||
#include <type_traits> | ||
|
||
/*! | ||
外部DLL読み込みをテストするためのテンプレートクラス | ||
CDllImp派生クラスを指定して使う。 | ||
テストではロード処理後の挙動をチェックするので、コンストラクタでinitしてしまう。 | ||
*/ | ||
template<class T, std::enable_if_t<std::is_base_of_v<CDllImp, T>, std::nullptr_t> = nullptr> | ||
class TExtModule : public T { | ||
private: | ||
std::wstring_view dllName = L""; | ||
|
||
protected: | ||
//! DLLパスを返す | ||
LPCWSTR GetDllNameImp(int index) override { | ||
return dllName.empty() ? T::GetDllNameImp(index) : dllName.data(); | ||
} | ||
|
||
public: | ||
//! コンストラクタ | ||
explicit TExtModule(std::wstring_view path = L"") | ||
: dllName(path) { | ||
// この関数の戻り値型はC++では推奨されない。 | ||
// あちこちで警告が出るとうっとおしいので呼出箇所をまとめておく | ||
// 将来的には、適切な戻り値型に変更したい。 | ||
this->InitDll(nullptr); | ||
} | ||
}; | ||
|
||
/*! | ||
外部DLLの読み込み失敗をテストするためのテンプレートクラス | ||
CDllImp派生クラスを指定して使う。 | ||
DLL読み込み失敗をテストするために「あり得ないパス」を指定する。 | ||
*/ | ||
template<class T> | ||
class TUnresolvedExtModule : public TExtModule<T> { | ||
private: | ||
// あり得ないパス | ||
static constexpr auto& BadDllName = LR"(>\(^o^)\<)"; | ||
|
||
// 基底クラスの型 | ||
using Base = TExtModule<T>; | ||
|
||
public: | ||
TUnresolvedExtModule() | ||
: Base(BadDllName) { | ||
} | ||
}; | ||
|
||
/*! | ||
外部DLLの読み込み失敗をテストするためのテンプレートクラス | ||
CDllImp派生クラスを指定して使う。 | ||
エクスポート関数のアドレス取得失敗をテストするためにMSHTMLを指定する。 | ||
MSHTMLは、Windowsには必ず存在していてサクラエディタでは使わないもの。 | ||
将来的にMSHTMLを使いたくなったら他のDLLを選定して定義を修正すること。 | ||
*/ | ||
template<class T> | ||
class TUnsufficientExtModule : public TExtModule<T> { | ||
private: | ||
// サクラエディタでは利用しないWindowsのDLL名 | ||
static constexpr auto& UnusedWindowsDllName = L"MSHTML.DLL"; | ||
|
||
// 基底クラスの型 | ||
using Base = TExtModule<T>; | ||
|
||
public: | ||
TUnsufficientExtModule() | ||
: Base(UnusedWindowsDllName) { | ||
} | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
/*! @file */ | ||
/* | ||
Copyright (C) 2022, Sakura Editor Organization | ||
This software is provided 'as-is', without any express or implied | ||
warranty. In no event will the authors be held liable for any damages | ||
arising from the use of this software. | ||
Permission is granted to anyone to use this software for any purpose, | ||
including commercial applications, and to alter it and redistribute it | ||
freely, subject to the following restrictions: | ||
1. The origin of this software must not be misrepresented; | ||
you must not claim that you wrote the original software. | ||
If you use this software in a product, an acknowledgment | ||
in the product documentation would be appreciated but is | ||
not required. | ||
2. Altered source versions must be plainly marked as such, | ||
and must not be misrepresented as being the original software. | ||
3. This notice may not be removed or altered from any source | ||
distribution. | ||
*/ | ||
#include <gtest/gtest.h> | ||
|
||
#include "TExtModule.hpp" | ||
|
||
#include "extmodule/CBregexpDll2.h" | ||
#include "extmodule/CHtmlHelp.h" | ||
#include "extmodule/CIcu4cI18n.h" | ||
#include "extmodule/CMigemo.h" | ||
#include "extmodule/CUchardet.h" | ||
//#include "extmodule/CUxTheme.h" //TSingletonなのでテストできない | ||
#include "macro/CPPA.h" | ||
//#include "plugin/CDllPlugin.h" //継承不可なのでテストできない | ||
|
||
/*! | ||
外部DLLの読み込みテスト | ||
*/ | ||
template <typename T> | ||
class LoadTest : public ::testing::Test { | ||
}; | ||
|
||
//! パラメータテストであるとマークする | ||
TYPED_TEST_CASE_P(LoadTest); | ||
|
||
/*! | ||
読み込み失敗のテスト | ||
DLLが見つからなくて失敗するケース | ||
*/ | ||
TYPED_TEST_P(LoadTest, FailedToLoadLibrary) | ||
{ | ||
// テスト対象のCDllImpl派生クラスをテストするための型を定義する | ||
using ExtModule = TUnresolvedExtModule<TypeParam>; | ||
|
||
// テストクラスをインスタンス化する | ||
ExtModule extModule; | ||
|
||
// DLLが見つからなくてIsAvailableはfalseになる | ||
EXPECT_FALSE(extModule.IsAvailable()); | ||
} | ||
|
||
/*! | ||
読み込み失敗のテスト | ||
エクスポート関数が取得できなくて失敗するケース | ||
*/ | ||
TYPED_TEST_P(LoadTest, FailedToGetProcAddress) | ||
{ | ||
// テスト対象のCDllImpl派生クラスをテストするための型を定義する | ||
using ExtModule = TUnsufficientExtModule<TypeParam>; | ||
|
||
// テストクラスをインスタンス化する | ||
ExtModule extModule; | ||
|
||
// エクスポート関数の一部が見つからなくてIsAvailableはfalseになる | ||
EXPECT_FALSE(extModule.IsAvailable()); | ||
} | ||
|
||
// test suiteを登録する | ||
REGISTER_TYPED_TEST_SUITE_P( | ||
LoadTest, | ||
FailedToLoadLibrary, FailedToGetProcAddress); | ||
|
||
/*! | ||
テスト対象(外部DLLを読み込むクラス) | ||
CDllImp派生クラスである必要がある。 | ||
*/ | ||
using ExtModuleImplementations = ::testing::Types< | ||
CBregexpDll2, | ||
CHtmlHelp, | ||
CIcu4cI18n, | ||
CMigemo, | ||
CUchardet, | ||
CPPA>; | ||
|
||
//! パラメータテストをインスタンス化する | ||
INSTANTIATE_TYPED_TEST_SUITE_P( | ||
ExtModule, | ||
LoadTest, | ||
ExtModuleImplementations); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
単体テストをしたいから本来は付けた方が良いキーワードを外すというのはあんまり良くない気がします。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
やろうとしてることは、
「実装の正当性を担保したいから」
「検証にあたって不都合なキーワードを外す」
です。
他の方法で「実装の正当性」を担保できているなら要らないです。
「本来は付けた方が良い」に関しては、
何故「付けた方が良い」になるか理解できていません。
final
キーワードを付けるとクラスが継承不可になるので、派生クラスに対するインターっフェースを定義する必要がなくなり、
クラス設計の大部分を省略できます。
(≒めんどくさい作業を省略できる)
「検証できないコードは不具合である」と仮置きして作業していますが、
final
キーワードを付けるメリットは、検証できないデメリットを補えるものなのでしょうか?継承を使わずにコードの検証ができる方法があれば、そっちに切り替えてもよいです。
手作業の検証だと人為的ミスの余地があるので自動テストが良いです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
出来れば非侵襲・非破壊で検査したいですがそうもいかないですよね。
FINALとかいうマクロは出来るだけ使いたくないので、継承しないでテスト出来る方法が無いか考えてみます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ぱっと見ですが
tests/unittests/TExtModule.hpp
ファイルのクラスを使わないでテストコードを書けばいいんじゃないかと思いました。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TExtModule.hpp
を使わなければ、final
を外さなくてもテストが書けるのは分かっています。tests1.exe
のカレントフォルダに"uchardet.dll"
という名前のDLLを用意すれば、コード記述量は多いですが「不可能ではない」と思います。
ステップ数も少ないですし、そこまでしてこのクラスをテスト可能にしたいとは思わないです。
自分は
final
を外すデメリットは「ない」と思っています。もしかして、そこに認識齟齬があるのかしら?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
もうちょっとコードを読んでみました。
継承して
GetDllNameImp
メソッドがoverrideして返す値を置き換える事で試験しているように見えます。FailedToLoadLibrary
はLoadLibraryが失敗するケースでのテストだと思いますが、パスが通っている場所にDLLが存在するとそれを読んで成功してしまうので、必ず失敗するように存在しないファイル名に置き換えているっぽいです。FailedToGetProcAddress
は本来使うのとは違うDLLを指定する事でGetProcAddressが失敗する確認を行っているように見えます。手法的にはお手軽にテストする為に本体側のコードを変更しているのが強引に感じるところです。まぁテストの為に private や protected を public にしたりもよくある話ですが…。
final を外すデメリットは現状無いのかもしれませんが、何らかの理由で継承をさせない為に付けたくなった場合にテストコードが継承を使っているから付けられない、という事態になりそうです。
まぁ継承を禁止しないと実装出来ないのか?というと必ずしもそうではないと思います。final はまぁ今のところ継承するつもりないしとりあえず付けとくか、みたいな感じで自分は使ってます。まぁ軽い意図なので変更しても問題は無いけど強引だなぁと感じるくらいです。
これは場合によって使い方というか逼迫度が異なると思います。
自分が付けた理由は、berryzplusさんのコメントにあるような、派生される事で想定しない挙動が生じうるのを防ぐ為にとりあえず指定、です。
ただ本当に真剣に継承されてしまうと問題が起きる(もしくは起きやすい)ケースで継承をさせたくないので final を付けるというケースはあるかもしれません。仮想関数によるpolymorphismとか使わないようなケースですね。あとは仮想関数の override を防ぐ為に final 指定する事も出来るので、その場合は override してほしくない強い意図を感じるのでテストしたいからといって外すのは良くない気がします。仮に外すならテスト用のビルドだけとかにした方がいいかなと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いまのところ、基底クラスも仮想メンバも持たないクラスや、パッと見では「十分に具象化された概念を表している」とは判断できないクラスに付けられた
final
を外すつもりはないです。final
を外してもメソッドのオーバーライドができないので外す意味がないです。「将来的にも継承させるつもりはない」の意思表示は「尊重すべき」と思います。final
は必要だと思います。単に「名前が悪い」という話ではなく、そういうケースはあると思います。この場合に「派生させたくない」という意思表示はfinal
でやるしかないと思います。「何故派生させたくないのか?」が少し気になりますが、あり得る話です。CUchardet.hのfinalを外した場合はどうか。
このクラスはパッと見で「十分に具象化された概念を表している」と判断できます。
finalを外すと「俺が作ったカスタムUchardet.dllを扱える専用クラス」という謎の派生品を作れるようになるのはデメリットですが、たぶん誰もやらんんので問題ないと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「十分に具象化された概念」というやや難しそうな言葉が登場しましたね。考えを整理する為に実装とは別の捉え方を導入すると解釈の違いで意思疎通が難しくなりそう…。
final を付けるべきか付けないべきかというのはプログラマ任せになりますが、判断基準は単に継承を許可したいかそれともしたくないかという事です。で、継承を許可したくない理由として継承に対応する事を考慮して実装をしていないから、で特に問題が無い気がします。それに対して何故というのを突き詰めて考えるのはテストコードを書く際にやる事ではない気がしますね。
結局のところberryzplusさんが実際にやっているのは、本来継承を想定していないクラスに対して、テスト方法として継承を使うようにする為に final を外している、です。継承せずにそれ以外の方法でも(手間は掛かりますが)行いたいテストは出来るわけなので、あんまり良くない、という感想に繋がります。
継承しない場合のテスト方法ですが、
継承禁止(final)を外すのを良くないと言われて「いやこのケースでは別に外すのは問題が無いんだ」というのがberryzplusさんの答えになっていると思いますが、本当に外すのが問題があるケースでは結局継承を使わずにテストする必要がありますよね?「その場合は継承使わないでテストするから問題ないよ」と言うのかもしれませんが…。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
継承を使わずにテストできる方法を思いつかないので、finalを外させてほしいです。
派生クラスの実装では上記2つを区別しなくてよいので、
FailedToGetProcAddresse
に相当するテストケースを用意すれば「final
を外さずにテストを実現する」ことは可能です。CUchardetの実装はこうなってるのでFailedToLoadLibrary
をテストする必要はないかも知れません。sakura/sakura_core/extmodule/CUchardet.cpp
Line 56 in e67c8ce
とりあえず、これが通らないと先に進めないことが問題だと思っています。
逆に、これが通ると #1815 で検証していたスタブDLLの仕組みを使って「外部DLLを読み込むクラス」に対する「スタブDLLを使ったテスト」を導入する流れにしていくつもりです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変更自体は許容出来るのでApproveしました。必要に応じてまた後から変更は出来ますし。
「継承を使わずにテストする方法は思いつかない」と書かれてますが、そのすぐ後に少し案が書かれているような気がします。
FailedToGetProcAddresse
については「DLL検索パスにカスタムのUchardet.DLLを置けば代替可能」と書かれていますがこれはGetProcAddressで読むシンボルが無いダミーのDLLファイルをDLL検索パスに置けばテスト出来るだろうという事ですよね?自分もその方法は思いつきました。そのDLLを作る記述を用意すれば出来ると思いますがちょっとその用意が億劫ですね。FailedToLoadLibrary
は仮にDLL検索パスに探すDLLファイルが存在してしまっていると、LoadLibraryでDLLが見つからないという挙動が再現出来ないという事ですかね?自分では色々と試してないですがそういう事かと想像しています。
WindowsAPIの
SetDllDirectory
に空文字を指定する事で、DLL検索パスからカレントディレクトリを取り除くことが出来るようです。そして下記の順にDLLを探すようです。という事で問題になりそうなのは、
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
https://docs.microsoft.com/ja-jp/windows/win32/dlls/dynamic-link-library-search-order#search-order-for-desktop-applications
https://docs.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications