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

CPPAクラスの自動テストを追加する #1817

Merged

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented Mar 9, 2022

PR の目的

CPPAクラスのテストを追加します。
自動テスト完成までの道のりが長そうなので、本体改修なしで実現できるテストを先に追加してしまいたいです。

カテゴリ

  • ビルド手順
    • ローカルビルド

PR の背景

#1722 で報告された CPPA::stdError の問題(?)に対処するため、自動テストを導入しようとしています。

自動テストを完成させるためには、本体側を改修しなくてはならないことがわかっています。

一括で導入するのが難しく、
かといって「少しずつ入れる」粒度も人によって基準がわかれそうなので、
いったん「本体側の改修を一切せずに導入できるテスト」を提案してみる次第です。

PR のメリット

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

仕様・動作説明

既存処理の正当性を担保するテストを追加します。
正当性の担保が目的なので、エラーになるケースは書いていません。

対象

  • CPPA::GetDllNameImp
    基底クラス CDllImpl のインターフェースで、読込対象のDLL名(パス)を返します。
  • CPPA::GetDeclarations
    CPPAの公開static関数で、PPA.DLLに登録する関数名をフォーマットします。

DLL読み込み失敗時の挙動(≒CDllImpl派生クラスの共通動作)については、
CPPAクラスのテストとは観点が異なるのでこのPRのテストとは別に準備します。

PR の影響範囲

自動テストのテスト件数が増えます。

テスト内容

関連 issue, PR

参考資料

本体改修なしで実現できるテストを実装。
@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2022

Kudos, SonarCloud Quality Gate passed!    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

TEST(CPPA, GetDllNameImp)
{
CPPA cPpa;
EXPECT_STREQ(L"PPA.DLL", cPpa.GetDllNameImp(0));
Copy link
Member

Choose a reason for hiding this comment

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

要件上は大小文字を区別しなくて良いので STRCASEEQ が適当に思えます。このままでももちろん問題はありません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

やってみました。
EXPECT_STRCASEEQ に変えるとビルドが通りません。
無理です。

このアサーションは本来、char* でも wchar_t* でも使えるように作ってあるはずです。
が、ここを書き換えたら wchar_t* を char* に変換できない というビルドエラーになりました。

gtestの不具合と考えられるのでフィードバックしたいですが、googletestは現在クローズド開発でC++11未対応の絶滅危惧種的コンパイラでもビルドできるようにする研究をしているらしいので無理っぽいです。(とても大事なことだと思いますが、一般ユーザーには縁のない話です。。。)

@berryzplus berryzplus marked this pull request as draft March 9, 2022 06:48
@berryzplus berryzplus marked this pull request as ready for review March 9, 2022 07:29
Copy link
Member

@kengoide kengoide 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 Author

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

@berryzplus berryzplus merged commit e67c8ce into sakura-editor:master Mar 10, 2022
@berryzplus berryzplus deleted the feature/add_test_of_cppa branch March 10, 2022 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants