CClipboard のテストにモックを導入する #1800

kengoide · 2022-02-13T08:03:11Z

PR の目的

CClipboard のテストにモックを導入し、プラットフォームのクリップボードへの依存を除去します。

カテゴリ

その他の問題

PR の背景

CClipboard の自動テストにモックを導入する検討を行っています。 #1798 により tests1.exe で Google Mock が使用できるようになりましたので、試験的に既存のテストをモック化したものを追加したいと思います。

プロジェクト内での実績がない類の変更であることもあり、当面は既存のテストと併存させる方向で検討中です。

仕様・動作説明

コミットを三分割しています。

CClipboard.h 編集時の再コンパイル範囲を縮小する。
CClipboard 内の API 呼び出しをオーバーライドできるようにする。
- 同じシグネチャの仮想メンバ関数を経由して API を呼び出すように変更するものです。
CClipboard のテストで Google Mock を使用する。
- 既存のテストのカバー範囲である SetHtmlText, SetText, GetText に対するテストを追加しています。

PR の影響範囲

CClipboard の既存コードに変更を加えますが、動作を変更するものではありません。

参考資料

* CDocEditor.h に実装されていた CClipboard の呼び出しコードを CDocEditor.cpp に移動。 * CDocEditor.h から #include "_os/CClipboard.h" を除去。 * 不足していた #include を追加。

* モックしたい関数を呼び出しているコードから名前空間指定を除去。 * モックしたい関数と同名の仮想メンバ関数をクラスに追加し、元の API へリダイレクトするように実装。

berryzplus

とりあえずコメントのみです。

berryzplus · 2022-02-13T08:19:21Z

sakura_core/_os/CClipboard.cpp

@@ -56,7 +56,7 @@ CClipboard::~CClipboard()

 void CClipboard::Empty()
 {
-	::EmptyClipboard();
+	EmptyClipboard();


ぬおっ！

やり方色々あるっす。

方法メリットデメリット

APIを自クラスのメンバとして定義する書き換えが楽。自クラスのメンバーがけっこう増える。

APIラッパークラスを定義してGetする自クラスのメンバー追加は1個で済む。ラッパークラスの定義内容でモメるかも。

APIラッパークラスを定義してシングルトンにするする自クラスのメンバーは増えない。ラッパークラスの定義内容でモメるかも。

たとえばこんな感じです。

CClipboardApi::getInstance()->EmptyClipboard();

berryzplus · 2022-02-13T08:29:34Z

tests/unittests/test-cclipboard.cpp

+
+// CF_HDROP を指定して取得する。
+TEST_F(CClipboardGetText, DISABLED_HDropSuccess) {
+	// 適切なダミーデータを用意するのが難しいため未実装


ここは一旦コレでよいと思いました。

AppVeyorBot · 2022-02-13T08:33:33Z

✅ Build sakura 1.0.4044 completed (commit f0b5309a9f by @k-kagari)

AppVeyorBot · 2022-02-13T09:46:19Z

✅ Build sakura 1.0.4045 completed (commit 7f18f51acb by @k-kagari)

berryzplus · 2022-02-13T21:37:14Z

雑に実装してみました。
なんか難しいですね・・・。
pull/1800/head...berryzplus:review/1800

気付いたこと

現状でCClipboardクラスのカバレッジは半分くらい。
スペルがおかしい（≒明確な仕様バグ）を持つ関数がいくつかある。
っていうかこのクラス、コンストラクタがおかしい。

berryzplus

GMockを使ってクリップボードの変更を抑止することはできていると思いました。

「あるべき」と思ったことの指摘を挙げましたが、対応は任意でよいです。

GMockを使うのと
GMockを正しく使うのは違うと思うので、
「意図通り機能しているならOK」で良いように思います。

berryzplus · 2022-02-14T00:37:30Z

sakura_core/_os/CClipboard.cpp

@@ -673,3 +673,19 @@ int CClipboard::GetDataType()
 	if(::IsClipboardFormatAvailable(CF_HDROP))return CF_HDROP;
 	return -1;
 }
+
+HANDLE CClipboard::SetClipboardData(UINT uFormat, HANDLE hMem) {
+	return ::SetClipboardData(uFormat, hMem);


このAPI関数ですが、クラスメンバを利用しないのでconst関数とすべきだと思います。

他のAPI関数も同様です。

const にしました。

berryzplus · 2022-02-14T00:43:40Z

sakura_core/_os/CClipboard.h

+
+protected:
+	// 単体テスト用
+	explicit CClipboard(bool openStatus) : m_bOpenResult(openStatus) {}


ここにexplicitを付けるなら、元々定義されているコンストラクタにも付けるべきと思いました。

コメントで書きましたが元のコンストラクタがおかしいので、ここに追加したコンストラクタは将来的に削除されることになると思っています。

既存のコードの改善を始める前にテストを整備したいので元のコンストラクタには手を付けていません。

失敗の可能性があるコンストラクタは static なファクトリー関数に置き換えるのが良いと考えています。クリップボードのオープンに失敗したら即座に呼び出し元に通知するべきです。状態を持つ必要がなくなるのでクラス内の条件分岐が減らせます。

このあたりの設計については各論あると思いますのでまたの機会に詳しく検討させてください。

berryzplus · 2022-02-14T00:46:48Z

sakura_core/_os/CClipboard.h

+	virtual HANDLE SetClipboardData(UINT uFormat, HANDLE hMem);
+	virtual HANDLE GetClipboardData(UINT uFormat);
+	virtual BOOL EmptyClipboard();
+	virtual BOOL IsClipboardFormatAvailable(UINT format);


コンストラクタ・デストラクタとメソッドの間には、なんらかの区切りが欲しいです。
ここに追加したメソッドはAPIに転送するものなおんで、constを付けるべきです。

追加するメンバ関数の前にコメントを追加しました。

berryzplus · 2022-02-14T00:49:34Z

tests/unittests/test-cclipboard.cpp

+	MOCK_METHOD2(SetClipboardData, HANDLE (UINT, HANDLE));
+	MOCK_METHOD1(GetClipboardData, HANDLE (UINT));
+	MOCK_METHOD0(EmptyClipboard, BOOL ());
+	MOCK_METHOD1(IsClipboardFormatAvailable, BOOL (UINT));


マクロMOCK_CONST_METHODnでconstメソッドを定義できるようです。

* 仮想メンバ関数を const にした。 * 仮想メンバ関数の宣言部に目的を説明するコメントを追加した。

sonarcloud · 2022-02-14T06:18:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

86.2% Coverage
0.0% Duplication

AppVeyorBot · 2022-02-14T06:28:31Z

✅ Build sakura 1.0.4047 completed (commit 24090e889b by @k-kagari)

kengoide · 2022-02-14T11:45:02Z

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

ghost · 2022-03-02T06:00:25Z

https://github.com/sakura-editor/sakura/runs/5355964382?check_suite_focus=true#step:6:115

GMOCK WARNING:
Uninteresting mock function call - taking default action specified at:
D:\a\sakura\sakura\tests\unittests\test-cclipboard.cpp(351):
    Function call: IsClipboardFormatAvailable(49512)
          Returns: 1
NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/master/docs/gmock_cook_book.md#knowing-when-to-expect for details.

ON_CALLというメソッドがあるようです。
ドキュメントでは必要な時のみEXPECT_CALLを用い、それ以外ではON_CALLを使うことが推奨されているようでした。
https://github.com/google/googletest/blob/main/docs/gmock_cook_book.md#knowing-when-to-expect-useoncall

berryzplus · 2022-03-02T15:36:29Z

すぐに次弾が出てくると予想してたので放置してました。
現状でやや問題あり（≒何か対処が必要）な認識です。
ただ、「バグか？」というと違うような気がしてます。

把握している問題点

👆で報告されている警告。
メッセージをよく読むと「無視してもいいよ」と書いてあるので問題なのか微妙。しかし、なんか気持ち悪い（笑
CClipboard関連のテストが、たまにコケる。
CClipboardのクラス内にカバーされていない行がけっこうある。
GlobalLock失敗時のコードがカバーされていない。
この部分は、定義意図不明なマクロで実装されているため、モック化する関数の選定から漏れたと考えられます。

kengoide · 2022-03-03T10:20:34Z

Uninteresting mock function call は ON_CALL を呼び出したメソッドに対しても表示されるものです。表示しないようにしたければログレベルを下げるという方法があります。コマンドラインフラグで指定できるようです。

CClipboard関連のテストが、たまにコケる。

これは把握していませんでした。

CClipboardのクラス内にカバーされていない行がけっこうある。
GlobalLock失敗時のコードがカバーされていない。

この2点は追加パッチで対処するつもりでしたが、個人的に余裕がない時期が続いており準備が進んでいません。

berryzplus · 2022-05-01T03:00:17Z

CClipboard関連のテストが、たまにコケる。

これは把握していませんでした。

すっかり忘れていましたが、自分の環境で「たまにコケる」原因は #1823 (comment) と考えられます。
※クリップボードの所有権取得は「先勝ち」になるので、クリップボードの所有権を取りに行くアプリでは他アプリによって所有権を取られている場合を考慮する必要があるっぽい、という話。

kengoide added 3 commits February 13, 2022 15:01

CClipboard.h 編集時の再コンパイル範囲を縮小する。

733f16e

* CDocEditor.h に実装されていた CClipboard の呼び出しコードを CDocEditor.cpp に移動。 * CDocEditor.h から #include "_os/CClipboard.h" を除去。 * 不足していた #include を追加。

CClipboard のテストで Google Mock を使用する。

a5a85a8

berryzplus reviewed Feb 13, 2022

View reviewed changes

protected コンストラクターを explicit にする

eca61cc

berryzplus previously approved these changes Feb 14, 2022

View reviewed changes

レビューコメントを反映

d2a0f72

* 仮想メンバ関数を const にした。 * 仮想メンバ関数の宣言部に目的を説明するコメントを追加した。

kengoide dismissed berryzplus’s stale review via d2a0f72 February 14, 2022 05:57

kengoide marked this pull request as ready for review February 14, 2022 06:29

berryzplus approved these changes Feb 14, 2022

View reviewed changes

kengoide merged commit e013bcd into sakura-editor:master Feb 14, 2022

This was referenced May 7, 2022

StdAfx.h に定義されている malloc と GlobalLock の型変換マクロを削除する #1842

Merged

CClipboard の単体テストをさらに追加する #1843

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CClipboard のテストにモックを導入する #1800

CClipboard のテストにモックを導入する #1800

kengoide commented Feb 13, 2022

berryzplus left a comment

berryzplus Feb 13, 2022

berryzplus Feb 13, 2022

AppVeyorBot commented Feb 13, 2022

AppVeyorBot commented Feb 13, 2022

berryzplus commented Feb 13, 2022 •

edited

Loading

berryzplus left a comment

berryzplus Feb 14, 2022

kengoide Feb 14, 2022

berryzplus Feb 14, 2022

kengoide Feb 14, 2022

berryzplus Feb 14, 2022

kengoide Feb 14, 2022

berryzplus Feb 14, 2022

sonarcloud bot commented Feb 14, 2022

AppVeyorBot commented Feb 14, 2022

kengoide commented Feb 14, 2022

ghost commented Mar 2, 2022

berryzplus commented Mar 2, 2022

kengoide commented Mar 3, 2022

berryzplus commented May 1, 2022

方法	メリット	デメリット
APIを自クラスのメンバとして定義する	書き換えが楽。	自クラスのメンバーがけっこう増える。
APIラッパークラスを定義してGetする	自クラスのメンバー追加は1個で済む。	ラッパークラスの定義内容でモメるかも。
APIラッパークラスを定義してシングルトンにするする	自クラスのメンバーは増えない。	ラッパークラスの定義内容でモメるかも。

CClipboard のテストにモックを導入する #1800

CClipboard のテストにモックを導入する #1800

Conversation

kengoide commented Feb 13, 2022

PR の目的

カテゴリ

PR の背景

仕様・動作説明

PR の影響範囲

参考資料

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 13, 2022

AppVeyorBot commented Feb 13, 2022

berryzplus commented Feb 13, 2022 • edited Loading

berryzplus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 14, 2022

AppVeyorBot commented Feb 14, 2022

kengoide commented Feb 14, 2022

ghost commented Mar 2, 2022

berryzplus commented Mar 2, 2022

kengoide commented Mar 3, 2022

berryzplus commented May 1, 2022

berryzplus commented Feb 13, 2022 •

edited

Loading