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

CProfileを拡張してテストに利用できるようにする #1546

Merged

Conversation

sanomari
Copy link
Contributor

PR の目的

タイトル通りです。

カテゴリ

  • リファクタリング

PR の背景

別件で検討中のコード修正を行う中で、単独で提案可能な改善を見つけたため、この部分だけ単独で提案する次第です。

PR のメリット

  • 独自定義のINIファイル入出力ユーティリティ CProfile を単体テストで利用できるようになります。

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

とくに思いつきません。

仕様・動作説明

文字列エントリを書き込むためのSetProfileDataを公開メソッドとして実装します。

従来のSetProfileDataImplはprotectedだったため、直接利用することができませんでした。

CDataProfile::IOProfileDataで文字列を書き込む場合にはstd::wstringを渡す必要があり、
設定値をリテラルで渡したいテストコードで利用するには不便でした。

PR の影響範囲

INIファイルの入出力を行うコードに影響します。
文字列値をINIファイルを書き込む処理で、セクションとキーと値を書けば簡単に出力できるようになります。
既存の単体テストで適当なファイルを作成する処理を簡潔に記述できるようになります。

テスト内容

同時に追加する単体テストだけで十分に検証できると思います。

関連 issue, PR

参考資料

@AppVeyorBot
Copy link

Build sakura 1.0.3462 completed (commit 4ae723ecd9 by @sanomari)

@sanomari sanomari marked this pull request as ready for review February 20, 2021 15:05
berryzplus
berryzplus previously approved these changes Feb 21, 2021
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.

大きな問題はなさそうです。

テスト内容

同時に追加する単体テストだけで十分に検証できると思います。

CDataProfileに未カバーコードとかTODOコメントとかあるのを踏まえたうえで「十分に検証できる」ですかね?

なんとなく、続き物の変更の一部を出している感じがして、めんどうな説明を端折ったように見えます。
(これこの後、CDataProfileのリファクタリングが来ますよね?たぶんw)

sakura_core/CProfile.cpp Outdated Show resolved Hide resolved
tests/unittests/test-cdlgprofilemgr.cpp Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

Build sakura 1.0.3464 completed (commit fd06670d2d by @sanomari)

@sanomari sanomari force-pushed the feature/use_cprofile_for_tests branch from 312f538 to 9792551 Compare February 21, 2021 06:13
@sanomari
Copy link
Contributor Author

CDataProfileに未カバーコードとかTODOコメントとかあるのを踏まえたうえで「十分に検証できる」ですかね?

未カバーコードは次の対応で消滅させます。
TODOコメントも次の対応で解消させます。

なんとなく、続き物の変更の一部を出している感じがして、めんどうな説明を端折ったように見えます。
(これこの後、CDataProfileのリファクタリングが来ますよね?たぶんw)

はい、その通りです。
以前出されていたCDataProfileのPRをベースに組み直したものを提案します。

@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2021

Kudos, SonarCloud Quality Gate passed!

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

84.6% 84.6% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.3465 completed (commit 385ec2499c by @sanomari)

@sanomari sanomari marked this pull request as ready for review February 21, 2021 06:57
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.

細かい指摘も対応していただきましたので、問題ないと思います。

ASSERT_EQ(109, nValue);

ASSERT_TRUE(cProfile.IOProfileData(L"Test", L"szTest", nValue));
ASSERT_EQ(0, nValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

現状の動きをトレースしたものですよね。

セクション"Test"のエントリ"szTest"には"value"が入っていて、
文字列"value"をint型に解釈することは不可能だと思います。

それなのに取得関数はtrueを返し、値は0に初期化されるという変な動きです。

@sanomari
Copy link
Contributor Author

レビューありがとうございます。次を準備します。

@sanomari sanomari merged commit 47944f7 into sakura-editor:master Feb 21, 2021
@sanomari sanomari deleted the feature/use_cprofile_for_tests branch February 21, 2021 08:03
@beru beru added the refactoring リファクタリング 【ChangeLog除外】 label Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring リファクタリング 【ChangeLog除外】
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants