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

DLL読み込み失敗のテストを追加する #1819

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sakura_core/_main/CNormalProcess.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,6 @@ class CNormalProcess final : public CProcess {

private:
CEditApp* m_pcEditApp; //2007.10.23 kobake
CMigemo m_cMigemo;
};
#endif /* SAKURA_CNORMALPROCESS_F2808B31_61DC_4BE0_8661_9626478AC7F9_H_ */
2 changes: 2 additions & 0 deletions sakura_core/extmodule/CHtmlHelp.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
#define SAKURA_CHTMLHELP_7003298B_3900_42FD_9A02_1BCD4E9A8546_H_
#pragma once

#include <HtmlHelp.h>

#include "CDllHandler.h"

/*!
Expand Down
2 changes: 1 addition & 1 deletion sakura_core/extmodule/CIcu4cI18n.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ typedef enum UErrorCode {
/*!
* ICU4C の i18n ライブラリ(icuin.dll) をラップするクラス
*/
class CIcu4cI18n final : public CDllImp
class CIcu4cI18n : public CDllImp
{
// DLL関数型定義
typedef UCharsetDetector* (__cdecl *ucsdet_open_t)(UErrorCode *status);
Expand Down
6 changes: 2 additions & 4 deletions sakura_core/extmodule/CMigemo.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,9 @@ typedef struct _migemo migemo;
#include "CDllHandler.h"
#include "util/design_template.h"

class CMigemo : public TSingleton<CMigemo>, public CDllImp {
friend class TSingleton<CMigemo>;
CMigemo(){}

class CMigemo : public CDllImp, public TSingleInstance<CMigemo> {
public:
CMigemo() = default;
virtual ~CMigemo();

// Entry Point
Expand Down
2 changes: 1 addition & 1 deletion sakura_core/extmodule/CUchardet.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ typedef struct uchardet * uchardet_t;
/*!
* uchardet ライブラリ(uchardet.dll) をラップするクラス
*/
class CUchardet final : public CDllImp
class CUchardet : public CDllImp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

単体テストをしたいから本来は付けた方が良いキーワードを外すというのはあんまり良くない気がします。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

やろうとしてることは、
「実装の正当性を担保したいから」
「検証にあたって不都合なキーワードを外す」
です。

他の方法で「実装の正当性」を担保できているなら要らないです。


「本来は付けた方が良い」に関しては、
何故「付けた方が良い」になるか理解できていません。

finalキーワードを付けるとクラスが継承不可になるので、
派生クラスに対するインターっフェースを定義する必要がなくなり、
クラス設計の大部分を省略できます。
(≒めんどくさい作業を省略できる)

「検証できないコードは不具合である」と仮置きして作業していますが、
finalキーワードを付けるメリットは、検証できないデメリットを補えるものなのでしょうか?

継承を使わずにコードの検証ができる方法があれば、そっちに切り替えてもよいです。
手作業の検証だと人為的ミスの余地があるので自動テストが良いです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

出来れば非侵襲・非破壊で検査したいですがそうもいかないですよね。
FINALとかいうマクロは出来るだけ使いたくないので、継承しないでテスト出来る方法が無いか考えてみます。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ぱっと見ですが tests/unittests/TExtModule.hpp ファイルのクラスを使わないでテストコードを書けばいいんじゃないかと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ぱっと見ですが tests/unittests/TExtModule.hpp ファイルのクラスを使わないでテストコードを書けばいいんじゃないかと思いました。

TExtModule.hppを使わなければ、final を外さなくてもテストが書けるのは分かっています。

tests1.exeのカレントフォルダに "uchardet.dll" という名前のDLLを用意すれば、
コード記述量は多いですが「不可能ではない」と思います。
ステップ数も少ないですし、そこまでしてこのクラスをテスト可能にしたいとは思わないです。

自分は finalを外すデメリットは「ない」と思っています。
もしかして、そこに認識齟齬があるのかしら?

Copy link
Contributor

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 はまぁ今のところ継承するつもりないしとりあえず付けとくか、みたいな感じで自分は使ってます。まぁ軽い意図なので変更しても問題は無いけど強引だなぁと感じるくらいです。

finalキーワードを付けるとクラスが継承不可になるので、
派生クラスに対するインターっフェースを定義する必要がなくなり、
クラス設計の大部分を省略できます。
(≒めんどくさい作業を省略できる)

これは場合によって使い方というか逼迫度が異なると思います。

自分が付けた理由は、berryzplusさんのコメントにあるような、派生される事で想定しない挙動が生じうるのを防ぐ為にとりあえず指定、です。

ただ本当に真剣に継承されてしまうと問題が起きる(もしくは起きやすい)ケースで継承をさせたくないので final を付けるというケースはあるかもしれません。仮想関数によるpolymorphismとか使わないようなケースですね。あとは仮想関数の override を防ぐ為に final 指定する事も出来るので、その場合は override してほしくない強い意図を感じるのでテストしたいからといって外すのは良くない気がします。仮に外すならテスト用のビルドだけとかにした方がいいかなと思います。

Copy link
Contributor Author

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を扱える専用クラス」という謎の派生品を作れるようになるのはデメリットですが、たぶん誰もやらんんので問題ないと思います。

Copy link
Contributor

@beru beru Mar 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「十分に具象化された概念」というやや難しそうな言葉が登場しましたね。考えを整理する為に実装とは別の捉え方を導入すると解釈の違いで意思疎通が難しくなりそう…。

final を付けるべきか付けないべきかというのはプログラマ任せになりますが、判断基準は単に継承を許可したいかそれともしたくないかという事です。で、継承を許可したくない理由として継承に対応する事を考慮して実装をしていないから、で特に問題が無い気がします。それに対して何故というのを突き詰めて考えるのはテストコードを書く際にやる事ではない気がしますね。

結局のところberryzplusさんが実際にやっているのは、本来継承を想定していないクラスに対して、テスト方法として継承を使うようにする為に final を外している、です。継承せずにそれ以外の方法でも(手間は掛かりますが)行いたいテストは出来るわけなので、あんまり良くない、という感想に繋がります。

継承しない場合のテスト方法ですが、

  • ファイル名は合っていてLoadLibraryが出来るけれどシンボルが無くてGetProcAddressに失敗するケース
    • そういうDLLファイルを作成すれば試験出来ます。しかし作成するのが手間なのでテストコードで継承を使っているように見えます。
  • ファイルが見つからなくてLoadLibraryが出来ないケース
    • 元々の実装で読み込むファイル名は決めた実装になっているのに、そこを無理やり変えてファイルが見つからない挙動のテストをしているのはちょっと強引です。
    • PATHが通っている場所に試験対象のDLLを置かないでテストコードを動かす必要がありますが、もしかしたらDLLによっては簡単にはいかないかもしれません。
  • DLLのファイルの内容が不正でLoadLibraryが出来ないケース
    • 今回思いつきましたが、こういう場合にLoadLibraryは失敗するはずです。

継承禁止(final)を外すのを良くないと言われて「いやこのケースでは別に外すのは問題が無いんだ」というのがberryzplusさんの答えになっていると思いますが、本当に外すのが問題があるケースでは結局継承を使わずにテストする必要がありますよね?「その場合は継承使わないでテストするから問題ないよ」と言うのかもしれませんが…。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

継承を使わずにテストできる方法を思いつかないので、finalを外させてほしいです。

  • FailedToLoadLibrary 現実的に代替方法が存在しません。
  • FailedToGetProcAddresse DLL検索パスにカスタムのUchardet.DLLを置けば代替可能。

派生クラスの実装では上記2つを区別しなくてよいので、FailedToGetProcAddresseに相当するテストケースを用意すれば「finalを外さずにテストを実現する」ことは可能です。CUchardetの実装はこうなってるのでFailedToLoadLibraryをテストする必要はないかも知れません。

return RegisterEntries(table);

とりあえず、これが通らないと先に進めないことが問題だと思っています。

逆に、これが通ると #1815 で検証していたスタブDLLの仕組みを使って「外部DLLを読み込むクラス」に対する「スタブDLLを使ったテスト」を導入する流れにしていくつもりです。

Copy link
Contributor

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を探すようです。

  • アプリケーションが読み込まれたディレクトリ
  • システム ディレクトリ
  • 16 ビット システム ディレクトリ
  • Windows ディレクトリ
  • PATH環境変数のディレクトリ
    • SetEnvironmentVariableでPATHを変更すればそのプロセスの分は制御出来そう

という事で問題になりそうなのは、CHtmlHelp のテストでしょうか? CHtmlHelp::GetDllNameImpHHCTRL.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

{
public:
// DLL関数ポインタ
Expand Down
101 changes: 101 additions & 0 deletions tests/unittests/TExtModule.hpp
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) {
}
};
104 changes: 104 additions & 0 deletions tests/unittests/test-extmodules.cpp
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);
2 changes: 2 additions & 0 deletions tests/unittests/tests1.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@
<ClCompile Include="test-crunningtimer.cpp" />
<ClCompile Include="test-csharedata.cpp" />
<ClCompile Include="test-czipfile.cpp" />
<ClCompile Include="test-extmodules.cpp" />
<ClCompile Include="test-printEFunctionCode.cpp" />
<ClCompile Include="test-cclipboard.cpp" />
<ClCompile Include="test-ccodebase.cpp" />
Expand Down Expand Up @@ -159,6 +160,7 @@
</ItemGroup>
<ItemGroup>
<ClInclude Include="eval_outputs.hpp" />
<ClInclude Include="TExtModule.hpp" />
<ClInclude Include="tests1_rc.h" />
</ItemGroup>
<ItemGroup>
Expand Down
6 changes: 6 additions & 0 deletions tests/unittests/tests1.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@
<ClCompile Include="test-cppa.cpp">
<Filter>Test Files</Filter>
</ClCompile>
<ClCompile Include="test-extmodules.cpp">
<Filter>Test Files</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="StartEditorProcessForTest.h">
Expand All @@ -177,6 +180,9 @@
<ClInclude Include="eval_outputs.hpp">
<Filter>Other Files</Filter>
</ClInclude>
<ClInclude Include="TExtModule.hpp">
<Filter>Other Files</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<ClInclude Include="tests1_rc.h">
Expand Down