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

テストカバレッジを少し改善する #1508

Merged

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Jan 17, 2021

PR の目的

今後のPR作成をやりやすくするために、テストカバレッジを少し改善する変更を行います。

カテゴリ

  • その他の問題

PR の背景

SonarCloud解析の結果を眺めていて、ソースコードの行数が想定より6万行ほど多いことに気付きました。

調べてみたら、解析対象が1ファイル余分でした。
この画像の一番下に出てる tests1-coverage.xml が余計です。
sonarcloud_code

このことから2点、修正したほうが良さそうな事案があることに気付きました。

  1. tests1-coverage.xml を除外したほうが良さそうです。
  2. 同じ場所に吐いているはずの tests1-googletest.xml が出力できていないです。
    なので、出力先を変更する必要がありそうです。

その他、GetDateTimeFormatのテストケースの一部が期待したチェックになっていないようなので暫定修正を入れます。

また、GetDateTimeFormatと同ファイルに実装されているCompareVersionについて、比較的簡単にテストを書くことができたので対応ついででテストケースを導入してしまいます。
https://sakura-editor.github.io/help/HLP000268.html#CompareVersion

PR のメリット

  • カバレッジに含めるべきでないファイルが解析対象から除外されます。
  • テストケースの一部が修正されて正しい状態になります。
  • 新たなテストケースが追加されます。

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

とくにないと思います。

仕様・動作説明

アプリ(=サクラエディタ)の仕様・機能に変更はありません。

PR の影響範囲

アプリコードを変更しないのでテストと解析結果のみに影響する変更です。

SonarCloud側の仕様についてはこのへんを参照してください。
https://docs.sonarqube.org/latest/analysis/coverage/

テスト内容

berryzplus/sakura を使って事前検証を行いました。

  1. テスト結果の出力先変更で、テスト結果が解析対象になること。
  2. 解析除外パターンの追加で、テスト結果とカバレッジが解析対象からはずれること
  3. coverage.cppの変更で、修正箇所がテスト未実施行にカウントされないこと
  4. GetDateTimeFormatに渡すフォーマット文字列の途中にNUL記号(\0)が含まれていた場合の分岐が実行されること。
  5. sakura_core/util/format.cpp のテストカバレッジが 100% となること

関連 issue, PR

#1504

参考資料

https://docs.sonarqube.org/latest/analysis/coverage/
https://sakura-editor.github.io/help/HLP000268.html#CompareVersion

@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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

@sanomari
Copy link
Contributor

個々の変更に文句はありませんが、いろいろ混ぜすぎの雰囲気を感じました。

テストケースの修正部分は非常に警備なので、
このテストを書いた @suconbu さんに依頼するのが良いかもしれません。

@suconbu
Copy link
Member

suconbu commented Jan 18, 2021

その他、GetDateTimeFormatのテストケースの一部が期待したチェックになっていないようなので暫定修正を入れます。

このテストを書いた @suconbu さんに依頼するのが良いかもしれません。

はい、喜んで😄
問題なければこの部分の修正 PR を作成します。

@sanomari
Copy link
Contributor

いろいろ混ぜすぎ

海外の大手プロジェクトでは、いろいろ混ぜすぎ、かつ、1コミットなPRが普通に通ってるので別にいいような気がしてきました。

個人的には、挙動が変わったときにどの関数のどういう条件の挙動が変わったかを検出できるテストケースが好みです。

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
思わず「こっちもか!」とつぶやきました。
とりあえずマージしちゃいます。

@berryzplus berryzplus merged commit 6975209 into sakura-editor:master Jan 19, 2021
@berryzplus berryzplus deleted the feature/improve_test_coverage branch January 19, 2021 11:17
@beru beru added the UnitTest label Mar 21, 2021
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