プロファイルマネージャを表示するかどうかの判定をテスト可能にする #1307

berryzplus · 2020-05-16T13:00:35Z

PR の目的

プロファイルマネージャを表示するかどうかの判定をテストできるようにリファクタリングし、実際に単体テストを追加します。

この対応を行うことにより、CProcessFactory::Create を単体テストから実行できるようにするための道が拓けます。

カテゴリ

リファクタリング

PR の背景

#1275 の対応により、単体テストコードで CSelectLang::InitializeLanguageEnvironment を実行させることが可能になりました。

PR のメリット

従来検証不能（≒妥当性の検証不能）だったコードの一部が検証可能になります。

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

このPRは仕様変更を含まない ただのリファクタリング です。
妥当性を検証できる と 妥当である は別の話なので、気になるとこあるけどスルーしています。
妥当性が検証できるようになることにより、現在のロジックの なんでやねん！ｗ ポイントが際立つ結果になると思います。

仕様・動作説明

仕様変更はありませんが、プロファイルマネージャの起動要否に関する仕様をまとめます。
https://sakura-editor.github.io/help/HLP000109.html

条件	判定	備考
`-PROFMGR` を指定	表示
`-PROF＝XXX` を指定	非表示	ヘルプの説明と矛盾しています。※1
`設定ファイルがない`	非表示	`リファクタリングで分岐を増やしました。`
`設定ファイルに適切なデフォルト設定がある`	非表示	設定の入出力がやや怪しいです。
`設定ファイルに適切なデフォルト設定がない`	表示

※1 ""でデフォルトを選択 という挙動は実装されていません。

従来のコードでは、条件分岐を検証しようとするとダイアログが起動してしまい、事実上検証不能でした。判定とダイアログ起動を分離することにより、ロジック単体の検証ができるようになります。

テスト内容

このPRの目的は単体テストができるようにすることなので、単体テストの実行結果で判断してください。

PR の影響範囲

アプリ（＝サクラエディタ）の機能に影響はありません。

参考資料

https://sakura-editor.github.io/help/HLP000363.html ヘルプ「プロファイルマネージャ」
https://sakura-editor.github.io/help/HLP000109.html ヘルプ「コマンドラインオプション」

AppVeyorBot · 2020-05-16T13:22:24Z

✅ Build sakura 1.0.2828 completed (commit 1c1cb1cdad by @berryzplus)

m-tmatma · 2020-05-16T23:10:27Z

処理のフローが変わっているので、関数が分離されているのをつくっつけて
差分がわかりやすいようにしたもののまとめ

before.txt
after.txt

m-tmatma · 2020-05-16T23:20:42Z

CDlgProfileMgr::ReadProfSettings( settings );

以下のコードの後に実行するように変更

	const int cchExeFileName = ::GetModuleFileName(NULL, szExeFileName, _countof(szExeFileName));
	CCommandLine::getInstance()->ParseKanjiCodeFromFileName(szExeFileName, cchExeFileName);

	CCommandLine::getInstance()->ParseCommandLine(lpCmdLine);

settings.m_nDefaultIndex == -1 の判定の代わりに、ReadProfSettings の戻り値が false かどうかで判定

0 < settings.m_nDefaultIndex の判定が 0 < settings.m_nDefaultIndex && settings.m_nDefaultIndex <= static_cast<int>(settings.m_vProfList.size()) に変更

0 < settings.m_nDefaultIndex の判定が成立しないときに pcCommandLine->SetProfileName( L"" ); を呼ぶ処理を削除

CSelectLang::InitializeLanguageEnvironment(); の呼び出し順番を移動し、bDialog が true の場合にのみ実行

CSelectLang::ChangeLang( settings.m_szDllLanguage ); を ReadProfSettings( settings ); が成功した場合のみ実行

CDlgProfileMgr dlgProf のインスタンス化順番を変更。使う直前に移動

m-tmatma · 2020-05-16T23:23:42Z

sakura_core/dlg/CDlgProfileMgr.cpp

+	SProfileSettings settings;
+	bool bSettingLoaded = ReadProfSettings( settings );
+
+	bool bDialog;


この変数はやめたほうがいいと思います。
~~関数に切り出したので、~~ なんのための変数かわかるようにした変数に変えて
かつ、戻り値の意味を関数のコメントに足したほうがいいです。

この変数はやめたほうがいいと思います。

その感覚には同意します。
レビュー観点は 変数名を再定義しないと単体テストを導入できないかどうか だと思います。
ぼくがやるとすると 別の機会でもいいんじゃね？ です。

なおこの変数は プロファイルマネージャ(＝ダイアログ)を表示しないと先に進めないかどうか を示しています。

コマンドラインの指定だけでプロファイルが判定できない場合、
ダイアログを表示して処理を止める必要があります。
なので、bDialogは分かりにくいけど間違った名前ではない、というのがぼくの見解です。

m-tmatma · 2020-05-16T23:24:37Z

sakura_core/dlg/CDlgProfileMgr.cpp

+		// プロファイル設定のデフォルトインデックス値から該当のプロファイル名が指定されたものとして動作する
+		pcCommandLine->SetProfileName( settings.m_vProfList[settings.m_nDefaultIndex - 1].c_str() );
+		bDialog = false;
+	}else{


オリジナルコードにあった
CCommandLine::getInstance()->SetProfileName( L"" ); を削っていますが、
なぜ不要になったんですか?

変更前のコードでは settings.m_nDefaultIndex == 0 のときに CCommandLine::getInstance()->SetProfileName( L"" ); が呼ばれていたが、新しいコードではなくなっている。

CCommandLine::getInstance()->SetProfileName( L"" ); を削っていますが、
なぜ不要になったんですか?

CCommandLine のコンストラクタが同等の処理を行っています。
不要になったというよりは、もともと不要なコードがあったのを削った認識です。

2つ目のコメントも関連するので一緒に書きます。

変更前のコードでは settings.m_nDefaultIndex == 0 のときに CCommandLine::getInstance()->SetProfileName( L"" ); が呼ばれていたが、新しいコードではなくなっている。

settings.m_nDefaultIndexの値域は 1 <= settings.m_nDefaultIndex <= settings.m_vProfList.size() なので、0は不正値（＝ダイアログを表示して選んでもらわないといけない）です。

既存コードには設定を読めなかった場合 のコードがなかったので、設定ファイルがなかったときと設定ファイルの値が不正だったときの処理が一緒になっていました。

設定ファイルがないのにプロファイルマネージャが出たら「は？」ってなるので、リファクタリング後のコードでは明示的に「ダイアログ表示不要」として何もしません。

設定ファイルがあるのにプロファイルが決まらなかったらプロファイルマネージャを出します。キャンセルで閉じない限り CCommandLine::getInstance()->SetProfileName( dlgProf.m_strProfileName.c_str() ); が呼ばれるので従来通り SetProfileName が呼ばれる結果になります。

m-tmatma · 2020-05-16T23:26:13Z

テスト内容

このPRの目的は単体テストができるようにすることなので、単体テストの実行結果で判断してください。

処理内容および順番に変更があるので、実際のユーザー操作で問題ないかのテストを行ってほしいです。

m-tmatma · 2020-05-16T23:32:02Z

sakura_core/_main/CProcessFactory.cpp

-		bDialog = true;
-	}else if( CCommandLine::getInstance()->IsSetProfile() ){
-		bDialog = false;
-	}else if( settings.m_nDefaultIndex == -1 ){


settings.m_nDefaultIndex == -1 の判定の代わりに、ReadProfSettings の戻り値が false かどうかで判定

}else if( 0 < settings.m_nDefaultIndex && settings.m_nDefaultIndex <= static_cast<int>(settings.m_vProfList.size()) ){

の判定が成立しないときに以下の処理があるので、変更前後で同等

}else{ // プロファイル設定のデフォルトインデックス値が不正なのでプロファイルマネージャを表示して設定更新を促す bDialog = true; }

認識あってます 😄
ゼロも不正値扱いにしてますけどね。

m-tmatma · 2020-05-16T23:37:07Z

sakura_core/_main/CProcessFactory.cpp

+	// コマンドラインオプションから起動プロファイルを判定する
+	bool profileSelected = CDlgProfileMgr::TrySelectProfile( CCommandLine::getInstance() );
+	if( !profileSelected ){
+		CDlgProfileMgr dlgProf;


CDlgProfileMgr はインスタンスが生成されても何もしないので、インスタンス順番を変更しても問題なさそう

CDlgProfileMgr はインスタンスが生成されても何もしないので、インスタンス順番を変更しても問題なさそう

コードの記述にはすべて意味があり、必要だからそこにあるはずです。

ここのif分岐に入らない場合にCDlgProfileMgr のインスタンスが必要ですか? [N/y]

まぁ、これを言い出すとプロファイルマネージャを表示するかどうかの判定ってコントロールプロセスにも必要？みたいな面倒な話をしないといけなくなっちゃうんですが。

細かい話はあとにして、とりあえず検証可能にしたい、がPRの目的です。

m-tmatma · 2020-05-17T10:20:06Z

tests/unittests/test-cdlgprofilemgr.cpp

+TEST( CDlgProfileMgr, TrySelectProfile_003 )
+{
+	// プロファイル設定を削除する
+	std::remove( "tests1_prof.ini" );


tests1_prof.ini は誰が作るのですか?

別のテストを実行したときに作られるのですか?
もしそうなら、テストの実行順番に依存するのでよろしくないです。

setup/teardown の仕組みを使って、テストの過程で
ローカルに作成した ini ファイルを掃除するとかした
ほうがいいと思います。

参考: http://opencv.jp/googletestdocs/primer.html#primer-test-fixtures

a40a52c 足しときました。

d774d8f で修正入れました。

tests1_prof.ini をテスト実行前に作成し、排他ロックをかけておくと失敗するようになっています。std::remove だと失敗時例外が出ないのね・・・。

AppVeyorBot · 2020-05-17T16:51:59Z

✅ Build sakura 1.0.2831 completed (commit acb8333759 by @berryzplus)

AppVeyorBot · 2020-05-17T17:14:10Z

✅ Build sakura 1.0.2832 completed (commit e0aff7a452 by @berryzplus)

m-tmatma · 2020-05-22T22:56:45Z

tests/unittests/test-cdlgprofilemgr.cpp

+	 * この名前は "%s_prof.ini" に 実行ファイル名 を埋め込んで生成される。
+	 * 実稼働環境では "sakura_prof.ini" となることに注意。
+	 */
+	static constexpr const char szProfileMgrIniName[] = "tests1_prof.ini";


実行ファイル名を、ファイル名にするという仕様なら、
固定値にせずに、 GetModuleFileName を使って取得するのがいいと思います。
その仕様のテストにもなりますし、テストが汎用的になります。

m-tmatma · 2020-05-22T23:52:16Z

tests/unittests/test-cdlgprofilemgr.cpp

+	 */
+	virtual void TearDown() {
+		// プロファイル設定を削除する
+		std::filesystem::remove( szProfileMgrIniName );


テスト後に szProfileMgrIniName の存在確認をしたほうがいいと思います。

これ対応してほしいと思ってますが、
別 PR でもいいと思うので approve しておきます。

要らんと思ったのでそのままにしときました。

参考
https://en.cppreference.com/w/cpp/filesystem/remove

条件結果

該当ファイルが削除できた何もおきない

該当ファイルが削除できなかった(存在していない) 何もおきない

該当ファイルが削除できなかった(ロックされている) 例外発生

該当ファイルがない何もおきない

削除できたかどうかで何かしたいわけではないので要らんと思います。
必要な理由に納得がいけば入れてもよいです。

berryzplus · 2020-06-11T08:15:05Z

レビューありがとうございます。
マージしてしまいます。

…_select_profile プロファイルマネージャを表示するかどうかの判定をテスト可能にする

…/add_try_select_profile" This reverts commit 74a0675, reversing changes made to 96cf722.

プロファイルマネージャを表示するかどうかの判定をテスト可能にする

d085040

m-tmatma reviewed May 16, 2020

View reviewed changes

m-tmatma reviewed May 17, 2020

View reviewed changes

berryzplus added 2 commits May 18, 2020 01:29

INIを使うテストのSetUp/TearDownを実装

a40a52c

ファイル削除に使う関数を削除例外が出るものに変更

d774d8f

m-tmatma reviewed May 22, 2020

View reviewed changes

m-tmatma approved these changes Jun 5, 2020

View reviewed changes

berryzplus merged commit 74a0675 into sakura-editor:master Jun 11, 2020

berryzplus deleted the feature/add_try_select_profile branch June 11, 2020 08:15

KENCHjp added the refactoring リファクタリング【ChangeLog除外】 label Jun 11, 2020

beru mentioned this pull request Jul 10, 2020

共通設定のウィンドウタブから他の設定タブに移動すると落ちる #1339

Closed

beru added a commit to beru/sakura that referenced this pull request Jul 10, 2020

Revert "Merge pull request sakura-editor#1307 from berryzplus/feature…

71a7189

…/add_try_select_profile" This reverts commit 74a0675, reversing changes made to 96cf722.

beru mentioned this pull request Jul 10, 2020

共通設定のウィンドウタブから他の設定タブに移動すると落ちる問題が起きないようにする #1340

Closed

berryzplus mentioned this pull request Aug 9, 2020

単体テストで起きたassert失敗を捕捉できるように、コンソールモードではメッセージボックスを表示しないようにしたい #1362

Merged

berryzplus mentioned this pull request Aug 30, 2020

TrySelectProfileテストを改善する #1384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

プロファイルマネージャを表示するかどうかの判定をテスト可能にする #1307

プロファイルマネージャを表示するかどうかの判定をテスト可能にする #1307

berryzplus commented May 16, 2020 •

edited

Loading

AppVeyorBot commented May 16, 2020

m-tmatma commented May 16, 2020

m-tmatma commented May 16, 2020 •

edited

Loading

m-tmatma May 16, 2020 •

edited

Loading

berryzplus May 17, 2020

m-tmatma May 16, 2020

m-tmatma May 16, 2020

berryzplus May 17, 2020

m-tmatma commented May 16, 2020 •

edited

Loading

テスト内容

m-tmatma May 16, 2020

berryzplus May 17, 2020

m-tmatma May 16, 2020

berryzplus May 17, 2020 •

edited

Loading

m-tmatma May 17, 2020

m-tmatma May 17, 2020

m-tmatma May 17, 2020

m-tmatma May 17, 2020

berryzplus May 17, 2020

berryzplus May 17, 2020

AppVeyorBot commented May 17, 2020

AppVeyorBot commented May 17, 2020

m-tmatma May 22, 2020

m-tmatma May 22, 2020

m-tmatma Jun 5, 2020

berryzplus Jun 11, 2020

berryzplus commented Jun 11, 2020

条件	結果
該当ファイルが削除できた	何もおきない
該当ファイルが削除できなかった(存在していない)	何もおきない
該当ファイルが削除できなかった(ロックされている)	例外発生
該当ファイルがない	何もおきない

プロファイルマネージャを表示するかどうかの判定をテスト可能にする #1307

プロファイルマネージャを表示するかどうかの判定をテスト可能にする #1307

Conversation

berryzplus commented May 16, 2020 • edited Loading

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

テスト内容

PR の影響範囲

関連 issue, PR

参考資料

AppVeyorBot commented May 16, 2020

m-tmatma commented May 16, 2020

m-tmatma commented May 16, 2020 • edited Loading

m-tmatma May 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented May 16, 2020 • edited Loading

テスト内容

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented May 17, 2020

AppVeyorBot commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Jun 11, 2020

berryzplus commented May 16, 2020 •

edited

Loading

m-tmatma commented May 16, 2020 •

edited

Loading

m-tmatma May 16, 2020 •

edited

Loading

m-tmatma commented May 16, 2020 •

edited

Loading

berryzplus May 17, 2020 •

edited

Loading