サクラエディタの起動テストを追加する

[berryzplus]

PR の目的

サクラエディタに起動テストを追加することにより、プログラム起動の成否に関わる不具合の混入を防止します。

• 起動テストは勝手に作った言葉です。
  単純にプログラムを起動して終了するテストを指しています。

カテゴリ

• その他の問題

PR の背景

#1363 スモークテスト導入に至るまでにある障害事項を提示する で書いている以下の問題の対策です。

GitHub移行後の大きな課題は レビューがテキトーなのでマージ後に不具合が発覚して追加修正を行うという事態がちょくちょく発生していること だと思います。

#1363 で作成したコードを吟味して1コミットにまとめてPRする次第です。

導入されると、少なくとも「プログラムの起動ができない」という系統の不具合をPR時点で検出できるようになります。

PR のメリット

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

仕様・動作説明

サクラエディタ自体の仕様はほとんど変わりません。

1. tests1.exeをサクラエディタとして起動できるようにする
   main関数を実装してtests1.exeの起動オプションを追加します。
   googletestのオプションと衝突させないため -PROF= が含まれる場合だけ wWinMain を呼ぶようにします。
2. tests1.exeを使ったテストで影響が出る箇所を修正する
   起動したコントロールプロセスの初期化完了を待機する処理を変更します。
   GUIプロセス専用関数である WaitForInputIdle を使うのをやめます。スモークテスト導入に至るまでにある障害事項を提示する #1363 (comment)
   コントロールプロセスを起動した直後に CreateMutex して存在確認していたのをやめます。スモークテスト導入に至るまでにある障害事項を提示する #1363 (comment)
   コントロールプロセスの初期化完了待ちのためのイベントをOpenしてたのをCreateするように変えます。スモークテスト導入に至るまでにある障害事項を提示する #1363 (comment)
   初期化完了イベントが取れなかったときに無視して続行していたのをやめます。
3. WinMainTest - runWithNoWin追加(コントロールプロセスの起動テスト)
   コントロールプロセスを起動して、終了するテストです。
4. WinMainTest - runEditorProcess追加(エディタプロセスの起動テスト)
   エディタプロセスを起動して、終了するテストです。

テスト内容

追加したテストが PASS になればOKです。

PR の影響範囲

コントロールプロセスが存在しない状態でエディタプロセスを起動しようとした場合の、
エディタプロセスが、起動したコントロールプロセスの初期化完了を待機する方法が変更されます。

関連 issue, PR

#1301 スモークテストを導入したい
#1363 スモークテスト導入に至るまでにある障害事項を提示する

参考資料

[AppVeyorBot]

✅ Build sakura 1.0.3047 completed (commit d3de83ae55 by @berryzplus)

[beru]

気になる点としてPRのタイトルは「サクラエディタの起動テストを追加する」なのでテストの追加を行うPRに見えますが、sakura_core/_main/CProcessFactory.cpp ファイルも変更しているので、実際にはテストの追加だけではないです。

既存のコードに動作上の問題があるので変更しているのでしょうか？動作上の問題は無いけれどテストをしやすくする為に変更しているのでしょうか？テストをしやすくする為だけの変更であれば、個人的な意見に過ぎませんがメリットを実感しにくいです。

あと変更が有るとその内容に問題が無いか確認しないといけないですが、これはまぁPRのレビューする以上しょうがないですね…。

[berryzplus]

気になる点としてPRのタイトルは「サクラエディタの起動テストを追加する」なのでテストの追加を行うPRに見えますが、sakura_core/_main/CProcessFactory.cpp ファイルも変更しているので、実際にはテストの追加だけではないです。

既存のコードに動作上の問題があるので変更しているのでしょうか？動作上の問題は無いけれどテストをしやすくする為に変更しているのでしょうか？テストをしやすくする為だけの変更であれば、個人的な意見に過ぎませんがメリットを実感しにくいです。

あと変更が有るとその内容に問題が無いか確認しないといけないですが、これはまぁPRのレビューする以上しょうがないですね…。

「仕様・動作説明」の2の変更のことを言ってると思います。

文字数多いので読むのたいへんだと思いますが、何故そうしてるかは書いてあるつもりです。

2の変更は、今回のPRの目的を達成するために必要だったので（≒起動テストを実現するにの不都合だったから）変更していますが、本質的には「もとの処理に不具合とまでは言えない軽微な誤りがあった」のを修正しています。

このPRは #1363 で作成したコードなので、元来は複数のコミットに分かれたものを結合して1コミットにして作成しています。コミットを分けたらどの修正行が 2 の変更にあたるか見やすくなるんですけど、そうすると「もとの処理に不具合とまでは言えない軽微な誤りがあったのを修正する」という独立した PR を作る衝動にかられる気がします。

sakura_core/_main/CProcessFactory.cpp の変更を手動で確認するのがほぼ不可能なのは見てわかると思います。
単独で出すと「不具合とまでは言えないなら直さなくったっていいじゃないか」が普通の反応だと思います。

起動テストを追加することにより、プログラム起動の成否に関わる不具合の混入を防止します。
そのために「不具合とまでは言えない軽微な誤り」を修正します、という構成です。

修正の影響は「エディタ起動時にコントロールプロセスを起動するときの初期化完了待機」なので、
問題があればWinMainTest - runEditorProcessの実行結果に影響が出ます。

[berryzplus]

PRを分ける・分けないは別にして、コミットを分けたほうが分かりやすかったりしますかね？

目的が理解できるか？目的を容認できるか？目的を達成しているか？が判断できればレビューは可能だと思いますし、医術的な検証は #1363 で終わってるので、コミットくっ付けてしまったんですが内容を理解するために分けた方がいいってことならコミットを分けてもよいです。

[beru]

今のタイトルや説明だとテスト追加がメインで２の変更が付随的なものに見えます。タイトルでテスト追加といいつつ本体の変更もついでにしてるのが何か違和感があるというか…

潜在バグが複数あるとしてそれへの対処をするならその修正にフォーカスしたPRの方が個人的にはわかりやすいです。

後から２の変更の説明に#1363のコメントへのリンクを張って済ませてますが、PRの説明でちゃんと書いた方が分かりやすいです。

課題に取り組んでる本人は圧縮率が高い記載で問題ないと感じても、意識を払ってない他人には分かりにくいという。。

まぁバカでも分かる解説書くなんてかったるくてやってらんねーよ、っていうのもそれはそれで全うな意見だと思います。

[berryzplus]

コミット分けますね。

[berryzplus]

ゼロから説明しなおしたほうが良さそうなので一旦変更を revert します。
説明しながらコミットを積んでいき、最終的に 9da897d に戻すイメージで進めていきます。
途中、テストがタイムアウトするのを見せながらやるので、本日中には完了しない予定です。

積み直しの1発目コミットは、起動テストの追加です。
プログラムの起動をテストするには、エントリポイントである wWinMain を呼び出してしまえばよいです。

もっとも、この方式にはいくつか問題があるため、色々対策が必要になって 9da897d が出来上がった経緯があります。
実際に何が起こるかを見てみないと判断つかないと思うので、 #1363 でやった実験を繰り返す感じです。

[AppVeyorBot]

❌ Build sakura 1.0.3098 failed (commit 8b8611e41e by @berryzplus)

[k-takata]

実際に何が起こるかを見てみないと判断つかないと思うので、 #1363 でやった実験を繰り返す感じです。

「不具合とまでは言えない軽微な誤り」を実演するのは重要ではなくて（#1363 を見れば何か問題が起きたらしいというのは分かりますから）、分析結果を示していただける方が理解が進むと思います。

• どういう誤りなのか
• どういう条件で問題が起こるのか（テストのみで起こるのか、通常使用でも条件さえそろえば起こるのか等）
• どういう方針で修正すべきと考えたか
• …

通常使用でも問題が発生しうるのであれば、確かに本体を直した方がよいだろうということになるでしょうし、通常使用では絶対に発生しないのであれば、テスト方法がよくないのではという疑問が出てきます。

[berryzplus]

「不具合とまでは言えない軽微な誤り」を実演するのは重要ではなくて（#1363 を見れば何か問題が起きたらしいというのは分かりますから）、分析結果を示していただける方が理解が進むと思います。

サクラエディタのプロセスには、エディタプロセスとコントロールプロセスがあります。

サクラエディタのコードの大半は、コントロールプロセスがINIファイルから作成した「共有メモリ」というプロセス空間にまたがって存在するグローバルデータに依存しています。このため、サクラエディタの実行中は基本的に、コントロールプロセスが実行されている必要があります。

エディタプロセスを起動しようとするとき、コントロールプロセスが起動されていない場合にはコントロールプロセスが起動されます。コントロールプロセスを起動しただけでは意味がないので、起動したコントロールプロセスが「共有メモリ」を作成してグローバルデータが使用可能になるまで待つようになっています。

「不具合とまでは言えない軽微な誤り」は、ここの待機のやり方（＝待機完了を検知するためのトリガー）がおかしいことを指しています。

誤りの影響は通常使用でも条件により発生するタイプで、実害は「エディタの起動が遅い」です。

エディタプロセスの開始時にコントロールプロセスが起動されていない場合のみに発生する事象ですが、人によっては毎朝体験している不具合かも知れません。発生メカニズムはあとで解説します。

[berryzplus]

977a210 のビルドでは、エディタプロセスの起動処理内でコントロールプロセスを起動しようとするコードが自分自身（＝ tests1.exe ）を起動させてしまう事象が起こっています。

• tests1.exe でテスト開始
  • tests1.exe を起動
    • tests1.exe を起動
      • tests1.exe を起動（無限に再帰）

テスト結果が成功と失敗に分かれている理由は、起動結果を検知していないことが原因です。
（その話はちょっとややこしいので、再帰起動の件を先に片づけます。）

対策としては、tests1.exe を「サクラエディタ」として起動できるようにすることを考えられます。
テストとして起動する機能を維持しつつ、エディタとして起動できるようにする、ということです。
gtest_man が定義しないサクラエディタ固有のパラメータを使ってこれを実現できると思います。

[AppVeyorBot]

❌ Build sakura 1.0.3099 failed (commit 9b5ccf2689 by @berryzplus)

[berryzplus]

tests1.exe を起動できるようにするだけでは問題は解決しませんでした。

• エディタ起動だけだと、コントロールプロセスが起動されていることの確認ができない。
• テストプロセス内から直接 wWinMain を呼び出すとグローバル変数が汚染されて他テストに影響が出る。
• コントロールプロセスが正常に起動されたかどうかを知るには、iniが作成されたかを見ればよい。
• エディタプロセスが正常に起動したことを知る方法はない。

なんとなく、問題とされてる部分はテストじゃないように思うので、このへんの課題解決の過程は割愛してしまいます。

次のコミットでサクラエディタ本体に加える変更以外を元に戻してしまいます。
（関係ないファイルを更新してしまってたのも元に戻します。）

[AppVeyorBot]

✅ Build sakura 1.0.3100 completed (commit 5366877491 by @berryzplus)

[beru]

誤りの影響は通常使用でも条件により発生するタイプで、実害は「エディタの起動が遅い」です。

コントロールプロセスの初期化に掛かった時間をテストで取得できるようにすれば、このPRの本体側の変更の効果が見えやすいです。

PRの説明やコメントで速くなったよ!!といわれて多分そうなんだろうとは思いますが…。

あとその本体側の変更を入れないとプログラムを起動して終了するテストが書けなくて成立しないなら、その理由、メカニズムをPRの説明に書いた方がいいです。

変更入れなくても成立するけど、コード見てて非効率な処理してて気になったからテストのついでに直しとく、という事ならそれは別PRにした方がいいと思います。

[berryzplus]

ビルド成功してしまってるので、問題点が超わかりづらいっすね・・・。
実行してみると分かるかも知れないです。

これ実は、エディタプロセスを起動するテストなのにエディタプロセスを起動できていないんです。

何故そうなるかというと、エディタプロセスが正常に起動できなくてもエディタは正常終了するからで、エディタプロセスが正常に起動できたかどうかを知る方法は存在しないからエディタプロセスを正常に起動できたかどうかをチェックしていないからです。

理由として挙げた問題点は、華麗にスルーします。

• エディタプロセスを正常に起動できない場合ってのは、実は他にもいくつかパターンがあります。そのいくつかのパターンをもれなく洗い出して、この場合はこう、あの場合はこう・・・とやっていく地味な作業をやりたいとは思わないですし、たぶんできないのでいっそのことスルーしてしまいます。
• エディタプロセスが正常に起動できたかどうかを知る方法は、既にあるコントロールプロセスの起動検出方法を参考に汲み上げたらよいと思います。ただし、多重起動できない仕様のコントロールプロセスと違って、エディタプロセスはいくらでも起動できるため若干の工夫が必要になります。現状で「参考にすべきはずのコントロールプロセスの起動検出方法」に何らかの問題がありそうだと分かってしまったのですぐに着手することは不可能だと判断してスルーしてしまうことにします。

[k-takata]

ビルド成功してしまってるので、問題点が超わかりづらいっすね・・・。
実行してみると分かるかも知れないです。

実演する必要はないと言っています。問題点をどう分析したかを示してください。

• エディタプロセスを正常に起動できない場合ってのは、実は他にもいくつかパターンがあります。

他のパターンはとりあえず置いておきましょう。今回のパターンにおいて、なぜ起動できないのか、どう修正することで起動できるようになるのかを示してください。

---

と、ここまで書いて今さらながら思ったのですが、ここでやろうとしているテスト（システムテストの一部と見做すべきでしょうか）は単体テストではありません。テストすべきはユーザーが実際に使う sakura.exe そのものであって、tests1.exe とやらではありません。仮に tests1.exe が動いても sakura.exe が動かなかったら何の意味もありません。sakura.exe そのものをテストする手段を考えるべきだと思います。

[berryzplus]

誤りの影響は通常使用でも条件により発生するタイプで、実害は「エディタの起動が遅い」です。

コントロールプロセスの初期化に掛かった時間をテストで取得できるようにすれば、このPRの本体側の変更の効果が見えやすいです。

測定しても無駄っす。

現象は「起動が遅れる」なんですけど「どれくらい遅れるか」が全く読めないからです。

全く読めない、の理由は、変更前が WaitForInputIdle だからです。

WaitForInputIdle は、待機対象のプロセスが開始したメッセージループが「開始後に初めて空振ったタイミング」を検出する関数です。この関数を使うと、他の人が書いた内部仕様が良く分からないプログラムを起動して待機するコードが簡単に書けます。待機する対象がGUIプログラムであれば表示したメインウインドウが初期表示のメッセージを食い尽くしてアイドル状態になる瞬間は必ず存在するので、有効な待機手段の一つだと思います。

ただし、より高度な待機機構であるイベントやミューテックスを利用できるようなら、そちらのほうが信頼性は高いです。コントロールプロセスのメッセージループが初めて空振るタイミングは、待機すべき処理（＝共有メモリの初期化）が完了するタイミングよりもだいぶ遅いので、ピンポイントで同期を発動できる同期オブジェクトを使うほうが有効です。

ちなみに WaitForInputIdle は 非GUI プロセスには使えません。

なので、コンソールプログラムである tests1.exe （サクラエディタとして起動モード）の初期化完了を WaitForInputIdle で待機することはできません。（待機しようとすると 失敗 を表すコードが返って即終了します。）

[berryzplus]

と、ここまで書いて今さらながら思ったのですが、ここでやろうとしているテスト（システムテストの一部と見做すべきでしょうか）は単体テストではありません。

当然ですが、分かっててやってる愚行です。

探しているのは、強引でも「単体テスト」を実現する方法であって、
システムテストをしたいわけじゃありません。（全機能網羅を目指すんですけど。

サクラエディタのグローバル依存はぼちぼち深刻です。
なので、wWinMainをテストから呼び出す以外の方法では通せないコードがあります。

そいつらを現代的なコードに組み替える方法は２つしかないです。
テストするのを諦めるか、強引にテストで包んで外部仕様を維持しつつ変えるかです。

テストするのを諦めるって方法は、初期にやって失敗してます。

代替策があればそれに乗っかる感じです。

[k-takata]

何のためにテストをするのか、テストの目的をはっきりさせましょう。PR の目的には

サクラエディタに起動テストを追加することにより、プログラム起動の成否に関わる不具合の混入を防止します。

と書いてありますが、これを目的とするならば単体テストの範疇ではないですし、sakura.exe を実際に起動してみるのが最も適切なテストです。

探しているのは、強引でも「単体テスト」を実現する方法であって、
システムテストをしたいわけじゃありません。（全機能網羅を目指すんですけど。

何のために単体テストを実行しますか？
単体テストの枠組みで無理やりシステムテスト（のような何か）を実行しようとしていませんか？
カバレッジを測定するのが目的であるなら、それはこの PR の目的とは異なるので、別のテストにすべきかもしれません。

[berryzplus]

これが修正のメインです。問題点の3つ目です。
動いているので「不具合とまでは言えない」ですが、やり方が間違っています。

このやり方を成立させるには、このコードが実行される前に、起動されたプロセスがCreateEventを完了している必要があります。イベント発動を待機するにあたり、イベント発動するスレッドがイベントを開いていることは必要条件ではないので、ノーチェックで CreateEvent してしまえばよいです。

[beru]

これは問題無さそうですね。

[berryzplus]

問題点の2つ目です。
このコードは「コントロールプロセスを起動した」ときにだけ呼ばれるコードです。
IsEistControlProcessで確認するのは「コントロールプロセスが既に起動されているかどうか」なので、ちょっと違うチェックをしています。エディタプロセス的にはコントロールプロセスが排他制御ミューテックスを作成しているかどうかに気にしなくてよいことです。（後続をOpenEventするためだけに必要。）

[berryzplus]

問題点の1つ目です。
WaitForInputIdleはプロセス同期を簡易化する目的で用意された便利関数なので機能に制限があります。

制限

• 対象が GUIプロセス でなければ利用できない。
• 対象が マルチスレッド である場合の挙動が保証されない。
• アイドル検出のタイミングを制御できない。

より高度な同期オブジェクトを使える場合にはそちらを使うべきで、ここのコードは既に同期オブジェクト（イベントオブジェクト）を利用しています。

[beru]

コントロールプロセスの初期化完了を後の処理でイベントを使って確認しているのでこのチェックは不要だという事ですね。

このチェックが有っても害は無さそうだけど、テストする都合上削りたい的な事なんじゃないかと思います。

WaitForInputIdle ってプロセスがユーザー入力を受け付けることができるアイドル状態になっているかを確認するAPIのようですが、コントロールプロセスが GUI を持つかというとタスクトレイのウィンドウを作ってメッセージループを回すので該当すると思います。

さてこのチェックが無くても本当に問題が無いのか？ですが、まぁ多分大丈夫なんじゃないでしょうか…。コントロールプロセスとエディタプロセス間でユーザー入力による待ち受けとかは無いので。

[AppVeyorBot]

✅ Build sakura 1.0.3101 completed (commit 122e18c478 by @berryzplus)

[berryzplus]

• 本体の修正が必要な理由を誰か少なくとも1人に納得させる。
  この場合、テストはとりあえずこのままでもいいでしょう。

とりあえず、だいぶ説明を足してみました。

• 本体の修正が不要なテストに変える。
  今のテストを少し修正するだけでいいのならそれがよいでしょう。が、個人的にはそうは思えないので、テストのやり方そのものを見直した方がよいと考えています。（望ましいのは sakura.exe をテストできるようにすることです。）

コンソールアプリとして起動したプロセスを、GUIプロセスと誤認させる手法があれば本体の修正は不要です。
何らかのシステムコールを呼ぶことで達成できるようなら、その方法を探してみるのもよいかも知れません。

[berryzplus]

「不具合とまでは言えない誤り」を修正しなければならない理由は分かってもらえたと思いますがどうでしょう？

[beru]

起動プロセス周りの変更は多分問題無いと思います。

テストコードですが tests1 プロジェクトのビルドで

1>ninja : error : rebuilding 'build.ninja': subcommand failed

というエラーメッセージが出てビルドに失敗したので他の人に任せます。

[beru]

@berryzplus さん

文字数多いので読むのたいへんだと思いますが、何故そうしてるかは書いてあるつもりです。

色々な解説のコメント書いていただいて理解しやすくなって助かりました。ありがとうございます。

自分の事は棚を上げてPRの説明に関して思う事を書きますが、これこれこういう変更をしたよの記述の後にその理由の解説を別チケットのコメントのリンクで済ますのは、あまり良くない気がします。PR単品で説明が完結していなくても問題が無いとなると、極端な事を言うとコメントのリンクを延々と辿ってからやっと解説に辿り付くようなものも有りえてしまうような…。

とはいえ物事の理解って1ページの文章で済ませられる事ばかりでは無いと思いますし、文章の書き方には多彩なアプローチがあると思うので、別にそのやり方が良くないと言いたいわけではありません。

[beru]

自分の事は棚を上げてPRの説明に関して思う事を書きますが、これこれこういう変更をしたよの記述の後にその理由の解説を別チケットのコメントのリンクで済ますのは、あまり良くない気がします。PR単品で説明が完結していなくても問題が無いとなると、極端な事を言うとコメントのリンクを延々と辿ってからやっと解説に辿り付くようなものも有りえてしまうような…。

とはいえ物事の理解って1ページの文章で済ませられる事ばかりでは無いと思いますし、文章の書き方には多彩なアプローチがあると思うので、別にそのやり方が良くないと言いたいわけではありません。

前の段落で 良くない気がします と書いておいて次の段落で そのやり方が良くないと言いたいわけではありません と書いているのも矛盾してますねｗｗ まぁ no-brainer ではないけど受け入れられなくは無いといったとこです。つまりそのやり方が悪いと決めつけたくは無いというか…。

[berryzplus]

テストコードですが tests1 プロジェクトのビルドで

1>ninja : error : rebuilding 'build.ninja': subcommand failed

というエラーメッセージが出てビルドに失敗したので他の人に任せます。

cmakeはconfigオプションを変えてリビルドする使い方に弱いみたいです。
設定を変えるには前回のconfigをクリアする必要があるっぽいです。

一度 git clean -dxf してみてください。
tortoiseGit の場合、詳細メニューの中にClean Up というコマンドがあります。
#1361 (comment)

コミットしてないファイルは消えてしまうので注意して下さい。

[beru]

このPRの変更内容が原因ではなく元からの挙動のようですが、サクラエディタ起動後にコントロールプロセスをタスクマネージャーで終了してから、残っているエディタプロセスで ファイル＞新規作成 すると、CProcessFactory::WaitForInitializedControlProcess() で呼び出す WaitForSingleObject がタイムアウトして新しいエディタの起動に失敗します。

コントロールプロセスを強制終了させる不正操作がイレギュラーで想定外なのでしょうがない気もしますが、そうなるメカニズムを追って調べてみました。

CProcessFactory::IsExistControlProcess() で正しくコントロールプロセスの有無を調べる事は出来ているようで、CProcessFactory::StartControlProcess() でコントロールプロセスの起動は行われていますが、すぐに終了してしまいます。CControlProcess::MainLoop() までに到達しないので CControlProcess::InitializeProcess() のどこかでメッセージを出さずに return false; しているようです。調べてみたところ CShareData_IO::ShareData_IO_Types で types.resize(pShare->m_nTypesCount); するとこで落ちていました。

タスクマネージャーでタスクを終了させる操作はWindows8以降は TerminateProcess のようで、強制終了されるコントロールプロセスが強制終了に介在出来る余地は無いようです。コントロールプロセスの2回目の起動で、CShareData::InitShareData() が呼ばれる際に CreateFileMapping 後の GetLastError の値が ERROR_ALREADY_EXISTS になる為に InitTypeConfigs( m_pShareData, *m_pvTypeSettings ); が呼ばれず m_pvTypeSettings の値が 0 のままになり、前の段落に書いた後に実行される処理でヌルポの参照のresizeのメソッドを呼び出して落ちてるっぽいです。

異常系でもリカバリする対策を入れるべきかというと、まぁあえて実装しなくても問題無いだろうなという気はします。

[beru]

cmakeはconfigオプションを変えてリビルドする使い方に弱いみたいです。
設定を変えるには前回のconfigをクリアする必要があるっぽいです。

一度 git clean -dxf してみてください。
tortoiseGit の場合、詳細メニューの中にClean Up というコマンドがあります。
#1361 (comment)

コミットしてないファイルは消えてしまうので注意して下さい。

過去の生成物が残っていてテストの新しいビルドを行う上で不整合があったんですね。tests フォルダの build と out ディレクトリを削除後に tests1 プロジェクトのビルドが問題無く動く事を確認しました。

対処方法を教えてくれて助かりました。ありがとうございます。

[beru]

テストを実行して WinMainTest のテストが PASS する事は確認しました。
CDlgProfileMgrIniTest.TrySelectProfile_003 が FAIL していますが自分の環境固有かもしれないのでこれについては無視します。

テストコードをざっと眺めてみましたが、test-winmain.cpp で関数名の接頭辞が CControlProcess_ のコントロールプロセス代わりの記述が気になります。本体側の実装のそのまま使っているわけでは無いので処理内容にどうしても差が出てしまうんじゃないかと思います。本体側の実装をモジュラーにしてテストでも再利用できるようにするか、それか通常の生成物の sakura.exe をブラックボックステストする別の方式のテストに切り替えた方が良いと思います。

あと起動テストとして具体的に何を確認していきたいのかがPRの説明に書かれていないので分かっていません。

TEST_F( WinMainTest, runWithNoWin ) では INI ファイルが作成される事をもって確認としています。

TEST_F( WinMainTest, runEditorProcess ) では ASSERT_EXIT で終了コードが 0 の確認と、ASSERT_TRUE でINIファイルが作成される事の確認をしています。

エディタが単純に起動するかどうかだけの話だったらPR作成者が事前にきちんとローカルでビルド後に実行すれば済む話だと思うので、それだけでは意義は無いと思います。エディタプロセス起動後に特定の処理や操作が問題無く行えるかの様々な確認が自動で行えるという事ならば、人がその都度労力を使って確認する手間を減らせるので省力化という事で意義があると思います。

そういえば過去にberryzplusさんがPR作成者で自分がPRのレビューアだった際に正常起動の確認さえもせずに master に merge 後に不具合の指摘が来た時がありましたがそれは単に両者の怠慢なので、単体テストで単純に起動するかの確認をしたいという話とは別ですね。

[berryzplus]

#1385 (comment) の件は、気付いていました。

対策としては CControlProcess の存在チェック方法や起動時の排他制御機構に手をいれなくちゃいかんくて、
前提知識なしで issue とか PR とか投げても思考停止するだけだろうと思ったので放置しておりました。

たぶん、何のためにコントロールプロセスを起動するのか？という話と、
コントロールプロセスは多重起動させたくない、という話を分けて考えないといけない気がします。

このPRで提案している、実際にコントロールプロセスを起動して待機してみるテストは、この問題を解決するためにも必要だと思っています。

[berryzplus]

テストを実行して WinMainTest のテストが PASS する事は確認しました。
CDlgProfileMgrIniTest.TrySelectProfile_003 が FAIL していますが自分の環境固有かもしれないのでこれについては無視します。

この件の対策を #1384 で出しています。
tests1_prof.ini が削除されないことがある、という問題があって、それを対策すれば発生しなくなるっす。

[berryzplus]

テストコードをざっと眺めてみましたが、test-winmain.cpp で関数名の接頭辞が CControlProcess_ のコントロールプロセス代わりの記述が気になります。本体側の実装のそのまま使っているわけでは無いので処理内容にどうしても差が出てしまうんじゃないかと思います。本体側の実装をモジュラーにしてテストでも再利用できるようにするか、それか通常の生成物の sakura.exe をブラックボックステストする別の方式のテストに切り替えた方が良いと思います。

本体側にある関数を直接利用していないのは、それらがprivateなメンバ関数であるために流用できないからです。
今回テストを導入するタイミングでリファクタリングを行うことが適切だとは思えなかったため、とりあえずテスト側に新規関数を定義しています。コメントに書いた説明のとおり、時期を見て本体側に戻すつもりです。

[berryzplus]

あと起動テストとして具体的に何を確認していきたいのかがPRの説明に書かれていないので分かっていません。

TEST_F( WinMainTest, runWithNoWin ) では INI ファイルが作成される事をもって確認としています。

TEST_F( WinMainTest, runEditorProcess ) では ASSERT_EXIT で終了コードが 0 の確認と、ASSERT_TRUE でINIファイルが作成される事の確認をしています。

目的に書いているとおり、起動できているかどうかを確認しています。

runWithNoWinではコントロールプロセスを起動して初期化完了を待機してから、非表示のメインウインドウを閉じてプロセスの終了を指示し、プロセスが実際に終了するまで待機します。この間に何か問題が起きれば例外が発生してテストが失敗するようになっています。（例外時の終了コードは非ゼロ）。iniの存在チェックはおまけで、なくても良いかもしれません。

コメントに書いていますがエディタプロセスが起動できたかどうかを知る方法はないため、 runEditorProcess ではエディタプロセスが起動するであろう コントロールプロセスの初期化完了を待機してから、コントロールプロセスが実際に終了するまで待機します。iniの存在チェックはおまけで、なくても良いかもしれません。

[berryzplus]

エディタが単純に起動するかどうかだけの話だったらPR作成者が事前にきちんとローカルでビルド後に実行すれば済む話だと思うので、それだけでは意義は無いと思います。エディタプロセス起動後に特定の処理や操作が問題無く行えるかの様々な確認が自動で行えるという事ならば、人がその都度労力を使って確認する手間を減らせるので省力化という事で意義があると思います。

手作業だと人によって Win32 - Debug しか見なかったり x64 - Release しか見なかったりすると思います。ぼちぼち3年目に入ると思うんですけど、CIビルドの信頼性はまあまあだと思っています。CIに組み込んでい置けばサポートする6種類のビルド設定(Win32/x64/MinGW, Debug/Release)で「なんとなくOK」の確認までできるので、省力化になると思っています。

そういえば過去にberryzplusさんがPR作成者で自分がPRのレビューアだった際に正常起動の確認さえもせずに master に merge 後に不具合の指摘が来た時がありましたがそれは単に両者の怠慢なので、単体テストで単純に起動するかの確認をしたいという話とは別ですね。

気を付けようね！というのとは別な話です。
CIビルドがコケてたら、きっと原因が気になって止めるでしょって発想です。

[beru]

本体側にある関数を直接利用していないのは、それらがprivateなメンバ関数であるために流用できないからです。

sakura_core/StdAfx.h に

#define private public

を入れるのだと駄目なんでしょうか？

[beru]

手作業だと人によって Win32 - Debug しか見なかったり x64 - Release しか見なかったりすると思います。

手作業で確認するのは x64 - Release だけでもとりあえず良いんじゃないかと思いますよ。不具合の内容によっては特定の種別のビルドでしか発生しないとかあると思いますが、そういう地雷を仕込んだPRを量産する能力を持つ人は稀有だと信じたいです。

PRの内容とか出す手間にもよりますが、起動してちょっとした操作が出来るかとか記述を変更した箇所が期待通りに動作しているかの確認ぐらいはPR出した人もレビューアーも行うべきだし、あとPR出した人は手元の変更を commit した後に push する前にローカルできちんと確認を行うべきですね。そうしないとレビューアがバグ取りをする羽目になります。#631 (comment) とか…。

ソフト開発者はプログラムで自動処理させる事を頑張る必要があるので手を抜けるとこには手間を省きたがるのは理解出来ます。

[beru]

#1385 (comment) の件は、気付いていました。

おーさすが各所の理解が深いですね。サクラエディタのソースコードは行数が多い上に大勢の人が関わっていて、とか色々な言い訳を適当に書き連ねてしまいそうですが自分は理解してないとこがかなり多いです。

対策としては CControlProcess の存在チェック方法や起動時の排他制御機構に手をいれなくちゃいかんくて、
前提知識なしで issue とか PR とか投げても思考停止するだけだろうと思ったので放置しておりました。

確かに手を入れている箇所が単純ではなかったり、人によっては興味が無い変更とかは食いつきが悪くてなかなかレビューがされないかもしれないですね。

たぶん、何のためにコントロールプロセスを起動するのか？という話と、
コントロールプロセスは多重起動させたくない、という話を分けて考えないといけない気がします。

前者は設計や仕様寄りの話で後者は必要な動作をどのようにして実現するかという実装寄りの話かもしれないですね。ところでコントロールプロセス無しで今の機能を実現出来ないかというとタスクトレイのウィンドウは別にしてなんか実現出来そうな気がぼんやりしました…。エディタプロセス1種類のみにしてあえて区別する作りにはしない方がもしかしたら良かったりするかも？？タスクトレイのウィンドウに関しても最初に起動したエディタプロセスが担当して、他のエディタプロセスが存在する状態でエディタのウィンドウが閉じられた場合にはエディタプロセスを閉じずにタスクトレイのウィンドウの為にプロセスを落とさない動作させるとか…。まぁ妄想だけでなく実際に調査や実装して確認しないと結論出せないですね。

このPRで提案している、実際にコントロールプロセスを起動して待機してみるテストは、この問題を解決するためにも必要だと思っています。

そうですねぇ、起動に関わる処理が動作している事を確認する手段の一つではあると思います。ただ成果物の動作確認方法としては、KENCHjpさんが #1301 (comment) で書かれてたソフトを使って、実際にユーザーが使う sakura.exe をチェックするアプローチの方が実装の詳細に引きずられる事が無いので良い気もします。「じゃあそのソフト使ったテストをお前が整備するんかい」と問い詰められたら「いやいやそこはチームワークで…」とかほざいてテストの手間が降りかかるのを避けようとすると思いますが…。

ところでプロセスやスレッド間の連携が隙が無く動作するかを頭の中で検証したり動作確認するのはややこしいのでシーケンス図やステートチャート図などがあると良いなと思います。まぁコードを見て理解しようとしない甘えかもしれませんが。

[beru]

目的に書いているとおり、起動できているかどうかを確認しています。

runWithNoWinではコントロールプロセスを起動して初期化完了を待機してから、非表示のメインウインドウを閉じてプロセスの終了を指示し、プロセスが実際に終了するまで待機します。この間に何か問題が起きれば例外が発生してテストが失敗するようになっています。（例外時の終了コードは非ゼロ）。iniの存在チェックはおまけで、なくても良いかもしれません。

コメントに書いていますがエディタプロセスが起動できたかどうかを知る方法はないため、 runEditorProcess ではエディタプロセスが起動するであろう コントロールプロセスの初期化完了を待機してから、コントロールプロセスが実際に終了するまで待機します。iniの存在チェックはおまけで、なくても良いかもしれません。

説明ありがとうございました。具体的にやっている事までをPRの説明に書いてほしいとか言い出すと、テストのソースコードまるまるコメントに貼り付けされても文句は言えないですね。

エディタプロセスが起動出来たかをどうかを知る方法はこのPRの変更で削られた WaitForInputIdle を使う方法はどうだろうか？とか思いましたが、それだときちんとした確認にはなってないかもしれないですね。

sakura.exe を起動後に少し待ってから色々やってアプリ操作してその結果を確認するテストコードでも良いような気がしますが、ポータブルな作りにするのが難しそうなのと、あとCI動作だとデスクトップが無いかもしれないのでGUIアプリの確認は難しいのかな。まぁ別にローカルで実行でも良い気はしますけど。

このPRでやっている事はサクラエディタ自体の起動テストではなくプロセス起動処理をテストに組み込んでの動作なので、そのアプローチの延長線でテストを増やしていくと、テストの都合でまた本体側を改造する羽目になりそうな気はします。

[beru]

テスト手法が気になりますがまぁ動いているので良いんじゃないでしょうか。

[berryzplus]

本体側にある関数を直接利用していないのは、それらがprivateなメンバ関数であるために流用できないからです。

sakura_core/StdAfx.h に

#define private public

を入れるのだと駄目なんでしょうか？

それでよい場合もあると思います。

短時間で元に戻せる確証があるときは、privateをpublicにしちゃうこともあります。
privateの使い方が全体的に間違ってるときとかはそうせざるを得ないかも知れないです。

今回はすぐに戻せないんで、また、全体的に間違ってるわけではないと思うのでやらなかったつもりです。

[berryzplus]

たぶん、何のためにコントロールプロセスを起動するのか？という話と、
コントロールプロセスは多重起動させたくない、という話を分けて考えないといけない気がします。

前者は設計や仕様寄りの話で後者は必要な動作をどのようにして実現するかという実装寄りの話かもしれないですね。ところでコントロールプロセス無しで今の機能を実現出来ないかというとタスクトレイのウィンドウは別にしてなんか実現出来そうな気がぼんやりしました…。エディタプロセス1種類のみにしてあえて区別する作りにはしない方がもしかしたら良かったりするかも？？タスクトレイのウィンドウに関しても最初に起動したエディタプロセスが担当して、他のエディタプロセスが存在する状態でエディタのウィンドウが閉じられた場合にはエディタプロセスを閉じずにタスクトレイのウィンドウの為にプロセスを落とさない動作させるとか…。まぁ妄想だけでなく実際に調査や実装して確認しないと結論出せないですね。

最終的には１種類だけにするのが分かりやすいと思うんですけどね。

２つ以上のプロセスを協調させる設計は、
それだけで「システム」を形成してしまって保守・開発の難易度がムダに上がるので。

エディタウインドウを管理する、というタスクを完全にエディタ側に委譲できれば実現可能と思っています。

[berryzplus]

エディタプロセスが起動出来たかをどうかを知る方法はこのPRの変更で削られた WaitForInputIdle を使う方法はどうだろうか？とか思いましたが、それだときちんとした確認にはなってないかもしれないですね。

WaitForInputIdleの制約事項に「待機対象がマルチスレッドだと使えない」があります。
https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-waitforinputidle

検索や描画のためのドキュメント解析をマルチスレッド化したい思惑があるので、それはもう使わない方向になると思います。利用箇所がいくつか残っているんですが、それらは「エディタプロセスの起動を検知できるようにする」を提案するときに置き換えるんじゃないかな？と思っています。

sakura.exe を起動後に少し待ってから色々やってアプリ操作してその結果を確認するテストコードでも良いような気がしますが、ポータブルな作りにするのが難しそうなのと、あとCI動作だとデスクトップが無いかもしれないのでGUIアプリの確認は難しいのかな。まぁ別にローカルで実行でも良い気はしますけど。

このPRでやっている事はサクラエディタ自体の起動テストではなくプロセス起動処理をテストに組み込んでの動作なので、そのアプローチの延長線でテストを増やしていくと、テストの都合でまた本体側を改造する羽目になりそうな気はします。

「テストの都合で本体側を変えたい」と言いそうな候補として「Grepダイアログ」があります。
本体側を変えなくてもやれないことはないのでやらないかもしれませんが、可能性はあると思います。

[berryzplus]

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