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

文字幅キャッシュのテストを追加する #1514

Merged

Conversation

kengoide
Copy link
Member

PR の目的

charset/charcode.cpp にある文字幅キャッシュ機構の自動テストを追加します。

カテゴリ

  • その他の問題

PR の背景

文字幅キャッシュとは、一度計算した文字の表示幅を保存して再利用する仕組みです。サクラエディタでは文字のレイアウト位置の計算に加え、半角と全角の区別や、文字種の判定にも利用しています。

用途が多いためコールグラフの奥深くでこれを呼んでいるコードパスが大量に存在しますが、この仕組みはグローバル変数に依存しており、間接的な依存関係を持つコードのテストが書きづらくなっています。なるべくグローバルな状態に依存しない形に書き換えていきたいため、作業の準備として現状をそのまま反映したテストを整備したい、というのが目的です。

PR のメリット

  • コードカバレッジが向上します。
  • リファクタリングが安全に行えるようになります。

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

  • 自動テストが GDI に依存するようになります。
    • 環境次第では失敗するかもしれません。
    • 他のAPIに切り替える際には手当てが必要です。

PR の影響範囲

テストコードのみの追加です。既存のコードに影響することはありません。

@kengoide kengoide force-pushed the feature/tests-for-char-width-cache branch from f680361 to 804e83e Compare January 23, 2021 18:15
@AppVeyorBot
Copy link

Build sakura 1.0.3368 completed (commit e32e1549b2 by @k-kagari)

@AppVeyorBot
Copy link

Build sakura 1.0.3369 completed (commit 5b26ac8c94 by @k-kagari)

sanomari
sanomari previously approved these changes Jan 24, 2021
Copy link
Contributor

@sanomari sanomari left a comment

Choose a reason for hiding this comment

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

いいですね。

実装も見てみましたが変なところはなさそうです。
これでいいの?を証明できるものは何もありませんが、きっと合ってそうです。

@berryzplus
Copy link
Contributor

PR のメリット

  • コードカバレッジが向上します。

https://github.com/sakura-editor/sakura/pull/1514/checks?check_run_id=1754762623

カバレッジは向上していないように見えます。
11.2% ⇒ 11.2%

なぜ向上しないかというと、テスト対象コードがエディタの初期表示の際に実行されるものだからです。現在のtests1.exeは、本来の単体テストでは絶対に実行できないコードを動かすテストが存在しています。なので、実行結果はチェックしていないけどコード実行自体は既に行っていたことになり、カバレッジが上がらない結果になったわけです。

テストコードの正当性という意味では、
'a'とか'あ'ではなくて、
「半角文字」と「半角以外の文字」で可能な限りの総当たりテストをやっとくのがベターだと思います。

berryzplus
berryzplus previously approved these changes Jan 24, 2021
@kengoide
Copy link
Member Author

kengoide commented Jan 24, 2021

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

これでいいの?を証明できるものは何もありませんが、きっと合ってそうです。

「これで本当にいいの?」と問われると歯切れのいい回答はできません…。

テストコードの正当性という意味では、
'a'とか'あ'ではなくて、
「半角文字」と「半角以外の文字」で可能な限りの総当たりテストをやっとくのがベターだと思います。

思案しましたが、どこまでやるのかについて納得のいく案が浮かびませんでした。ローカルでカバレッジをとる作業を怠ったせいで今回のテストでは通らないままになってしまったパスがありまして、それらに対するテストを追補することで実効性を高めていきたいと思います。

@kengoide kengoide dismissed stale reviews from berryzplus and sanomari via f0b8aab January 24, 2021 11:43
@kengoide
Copy link
Member Author

kengoide commented Jan 24, 2021

あ、conflict解消すると再レビュー要るんですね…。rebaseします。

@kengoide kengoide marked this pull request as draft January 24, 2021 11:46
@kengoide kengoide force-pushed the feature/tests-for-char-width-cache branch from f0b8aab to 54238ca Compare January 24, 2021 11:51
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AppVeyorBot
Copy link

Build sakura 1.0.3375 completed (commit 274b8f8157 by @k-kagari)

@AppVeyorBot
Copy link

Build sakura 1.0.3376 completed (commit 555b75689f by @k-kagari)

@kengoide kengoide marked this pull request as ready for review January 24, 2021 12:35
@kengoide
Copy link
Member Author

tests1.vcxproj.filters のコンフリクトの解決を行いました。お手数ですが再レビューをお願いします。

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.

問題ないと思います。

従来は実行はしても結果の確認はしてなかったので、入れる意味は十分にあると思っています。どうだったら正当なのか?は別な話かも知れません。

@kengoide kengoide merged commit 615d19c into sakura-editor:master Jan 24, 2021
@kengoide
Copy link
Member Author

マージしました。ありがとうございました。

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.

5 participants