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

CWordParseのテストを追加する #1488

Merged
merged 5 commits into from
Jan 14, 2021

Conversation

kengoide
Copy link
Member

@kengoide kengoide commented Dec 16, 2020

PR の目的

CWordParseの関数群に対するテストを追加します。

カテゴリ

  • その他の問題

PR の背景

#1470 で変更を加えたCWordParseに対してテストを追加するPRです。

PR のメリット

  • CWordParse::WhatKindOfCharCWordParse::WhatKindOfTwoCharsが何を判定しているかがテストコードに明文化されます。

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

  • CWordParse::WhereCurrentWord_2の引数が増え、呼び出しが煩雑になります。

仕様・動作説明

  • CWordParse::WhereCurrentWord_2が共有データに依存していたため、データを関数の引数として受け取るよう変更し、呼び出し側に依存関係を移動しています。単純に移動しただけですからバグの混入は考えづらいのですが、呼び出しているコードパスが関係する機能について念のための手動テストが必要になるかもしれません。
  • CWordParse::WhatKindOfCharのテスト対象は条件式で判定している既知のコードポイントに限定しています。不明なコードポイントの文字幅をGDIに問い合わせるコードパスが存在しますが、テスト対象に含めるとグローバル変数であるフォントキャッシュの初期化が必要になること、やることが単体テストからかけ離れていくことを理由として無視しています。

PR の影響範囲

CWordParse::WhereCurrentWord_2内で参照していた[共通設定]→[編集]→[改行コードNEL, LS, PSを有効にする]フラグを関数外に移動したため、各機能に関連するコードが壊れている可能性があります。

  • ダブルクリックでの単語選択
  • ダブルクリック&ドラッグによる単語単位の選択
  • 単語単位の検索
  • 検索ダイアログの初期文字列(現在キャレット位置の単語が自動で入力される)
  • [編集]→[高度な操作]以下の単語関連機能
  • 入力補完(Ctrl+Space)

(他にもあるかも…。上記が代表例だと思いますが)

テスト内容

上記の各機能の挙動に異常がないことを確認します。

関連 issue, PR

#1470 コンボボックスに Ctrl+Backspace による単語削除機能を追加する

@kengoide
Copy link
Member Author

kengoide commented Dec 16, 2020

MinGW失敗してますね。サロゲートペアだめなのかな…。
いや、UTF-8なのにサロゲートペアも何もないですね。何でだ?

@AppVeyorBot
Copy link

Build sakura 1.0.3299 completed (commit cb123df245 by @k-kagari)

@kengoide kengoide marked this pull request as draft December 16, 2020 14:08
@kengoide
Copy link
Member Author

いったんDraftにします。

CFLAGS= \
-finput-charset=utf-8 \
-fexec-charset=cp932 \

原因これかな?

-fexec-charset=charset

Set the execution character set, used for string and character constants. The default is UTF-8. charset can be any encoding supported by the system’s iconv library routine.

-fwide-exec-charset=charset

Set the wide execution character set, used for wide string and character constants. The default is UTF-32 or UTF-16, whichever corresponds to the width of wchar_t. As with -fexec-charset, charset can be any encoding supported by the system’s iconv library routine; however, you will have problems with encodings that do not fit exactly in wchar_t.

-fwide-exec-charset=utf-16le で通りそう? デフォルトでUTF-32 or UTF-16だと書いてあるのに指定が必要というのも不可解ではありますが。

@AppVeyorBot
Copy link

Build sakura 1.0.3300 completed (commit 71094ea207 by @k-kagari)

@AppVeyorBot
Copy link

Build sakura 1.0.3301 completed (commit a754bfd45a by @k-kagari)

@AppVeyorBot
Copy link

Build sakura 1.0.3302 completed (commit add012aba7 by @k-kagari)

@kengoide
Copy link
Member Author

MinGW通りました。Draft外します。

@kengoide kengoide marked this pull request as ready for review December 16, 2020 15:36
@berryzplus
Copy link
Contributor

前に自分が指摘されて「アフォか!」と思った趣旨の指摘をあえて書きます。
(あとになって考えてみると妥当な主張だと思っているので、おそらく正しい指摘です。)

本体を変更しなくても実現できるテストとそうでないテストを区別できるようにコミットを分けて欲しいです。

CWordParseを「テスト可能にするために変更している」わけですが、
全部の関数が「変更しないとテストできない」わけではないと思います。
PRの説明にあるとおり、変更したことによって壊す可能性はゼロじゃないので、
「テストのための変更」がテスト前実装の挙動を変更してないことを保証する意味でも
「変更しなくてもテスト可能な部分」を先にテスト化したほうがいい気がします。

例によって CodeFactor は「複雑度が高い」とかゆうクソ分かりづらい指摘のみなのでスルーでよいと思います。
スルーすることが事実上確定しているのだから、CodeFactor外したらいいんじゃないかと思うんですけど、
まぁ、それは別な話かも知れません。(CodeFactorが現出している有用っぽい指摘に対応したい気持ちもちょっとある。)

@kengoide
Copy link
Member Author

コミットを分割しました。

例によって CodeFactor は「複雑度が高い」とかゆうクソ分かりづらい指摘のみなのでスルーでよいと思います。
スルーすることが事実上確定しているのだから、CodeFactor外したらいいんじゃないかと思うんですけど、
まぁ、それは別な話かも知れません。(CodeFactorが現出している有用っぽい指摘に対応したい気持ちもちょっとある。)

個人的には関数を分割しつつテストを追加していく作業に手を出したい気持ちがあるのですが、「この関数分割したぜ!」という内容のPRをレビューする側の負担を考えるとそれもどうかなあ…?と思っているところです。

@AppVeyorBot
Copy link

Build sakura 1.0.3303 completed (commit 7c5b56402d by @k-kagari)

@kengoide kengoide added the Test label Dec 17, 2020
@AppVeyorBot
Copy link

Build sakura 1.0.3307 completed (commit 943b9c76cf by @k-kagari)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

問題なしで良いと思います。

一応、SonarCloudのテストリポジトリで解析してみました。
問題なし、で片づけてよい結果だと思います。
sakura-editor/SakuraSonarTest#20

@kengoide
Copy link
Member Author

レビューありがとうございます。
PRの静的解析結果がコメントで報告されるのは劇的な進歩だと思います。ご尽力に感謝します。

@kengoide kengoide merged commit 9fe55d8 into sakura-editor:master Jan 14, 2021
@kengoide kengoide deleted the feature/tests-for-cwordparse branch January 14, 2021 03:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants