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

append unit testing framework #121

Merged

Conversation

qwerty2501
Copy link
Contributor

@qwerty2501 qwerty2501 commented May 3, 2022

内容

projectにunit test frameworkを導入しました。たたき台的な感じです。

関連 Issue

ref VOICEVOX/voicevox_project#1 (comment)

その他

とりあえず導入を急ぐのと、どういう書き口で書けるのか見せるため実装してるテストケースは一つのみです。テストケースの充実は各々その機能を実装するときにでもやっていただければと思います。
Catch2で書きましたが、とりあえずコード見たほうが議論しやすいだろうと思って書いたものであり、Catch2採用が決定してるわけではないです
またCIでのテスト実行もこのPRではやりません。PR分けたほうが良さそうなので
リリース時にはテスト実行は不要なのでCIに引数を追加してます

テストコードの配置場所についてですが、とりあえず tests/unit_tests の下に実装が書いてあるソースコードに対応する感じのディレクトリ構成にしました

念の為レビュー時にWindowsでもテスト実行できることを確認していただけますでしょうか
またVSCodeにCatch2向けの拡張 があるようです

FetchContent_Declare(
Catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v3.0.0-preview5)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

完全に余談ですがCatch2のドキュメントでcmakeのFetchContent機能を知りました。
このプロジェクトが依存してるopenjtalkもcmakeで作られてるようなので、やろうと思えばopenjtalkもFetchContentでとってくるようにし、git submoduleの依存から脱却できそうだなと思いました。
もっとも将来的にcmakeを使ってないライブラリに依存するようになった場合はまたgit submoduleを使わないといけなさそうですが

@Hiroshiba
Copy link
Member

PRありがとうございます!!

core周りで頼りになる方にレビューをお願いできるととても心強いです。
もしよかったらレビューお願いできると・・・! @Yosshi999 @Oyaki122

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@Yosshi999 Yosshi999 left a comment

Choose a reason for hiding this comment

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

変更点と質問を書きました。僕の環境はWindowsのVisual Studio 16.11.2です

あともう一点:
Debugビルドでunit_testをビルドしようとするとcoreのコンパイラオプション /O2 と衝突してそうです。単体テストをReleaseビルドでやるのはおかしいですかね?その場合coreのCMakeListsをいじる必要がありそうです。

cl : コマンド ライン error D8016: コマンド ライン オプション '/O2' と '/RTC1' は同時に指定できません [C:\Users\Orange\Desktop\game\voicevox_core\test_build\core\core.vcxproj]

tests/CMakeLists.txt Show resolved Hide resolved
target_include_directories(unit_test PRIVATE ${Catch2_SOURCE_DIR}/src)
target_include_directories(unit_test PRIVATE ${CORE_DIR}/src)
target_link_libraries(unit_test PRIVATE Catch2::Catch2WithMain)
target_link_libraries(unit_test PRIVATE core)
Copy link
Contributor

Choose a reason for hiding this comment

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

[QUESTION] これ僕の環境だとリンクに失敗するんですが他の方再現しますか?

kana_parser_test.obj : error LNK2019: 未解決の外部シンボル "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl voicevox::core::engine::e
xtract_one_character(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,unsigned __int64,unsigned __int64 &)" (?extract_one_ch 
aracter@engine@core@voicevox@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV45@_KAEA_K@Z) が関数 "void __cdecl CATCH2_INTERNAL_TEST_0(void)" (?CATCH2_
INTERNAL_TEST_0@@YAXXZ) で参照されました 

Copy link
Contributor Author

@qwerty2501 qwerty2501 May 7, 2022

Choose a reason for hiding this comment

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

オプションを修正してみましたが、これでだめなら coreのCMakeLists.txtでcore targetがSHARED指定されてるからかもしれないです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

エラーメッセージ的に標準ライブラリとのリンクに失敗しているようだったので、unit_testとcoreの標準ライブラリのバージョンをそろえてみました

Copy link
Member

Choose a reason for hiding this comment

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

修正後のコードでコンパイルしてみたところ、同じエラーが出たっぽいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Catch2が原因ぽそうだけど直せるだろうか・・・

Copy link
Contributor

Choose a reason for hiding this comment

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

おそらくcoreがextract_one_characterをexportしてないせいでリンクが通らないので、
coreのcmakelistsに set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) を追記することですべての関数をexportさせるか、
static版も作ってそっちにリンクする必要があるのだと思います。

またshared版はもう一つ課題があって、リンクが通ったとしても今度はcore.dllとonnxruntime.dllをunit_testのバイナリがあるところにコピーしてくる必要があります。これはcmakeでやるのは結構厳しくて、cmake 3.21の依存dllを引っぱってくる機能

  add_custom_command(TARGET unit_test POST_BUILD
    COMMAND ${CMAKE_COMMAND} -E copy
    $<TARGET_RUNTIME_DLLS:unit_test> $<TARGET_FILE_DIR:unit_test>
    COMMAND_EXPAND_LISTS )

ではcore.dllしか拾ってくれません。(ちなみにcore.dllを持ってくるだけなら3.21に上げる必要は無く、 $<TARGET_FILE:core> がcore.dllへのパスを指してるのでそれを -E copy するだけで足ります)

つまり現状core/CMakeLists.txtで定義したONNXRUNTIME_LIBSとかをこっちに持ってきて別途 -E copy する必要があります。

Copy link
Contributor

Choose a reason for hiding this comment

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

Yosshi999@7b4d430
一応これでビルドが通る?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Yosshi999 そのcommitを参考に修正しました

Copy link
Member

Choose a reason for hiding this comment

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

問題なくテストできてそうなことを確認しました!!

1/1 Test #1: extract_one_character ............ Passed 0.02 sec

Copy link
Contributor

Choose a reason for hiding this comment

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

こちらでも動作確認できました!

@qwerty2501
Copy link
Contributor Author

@Yosshi999 ConfigがReleaseの場合のみに最適化オプションが有効化されるように修正しました

tests/CMakeLists.txt Outdated Show resolved Hide resolved
@PickledChair
Copy link
Member

PickledChair commented May 10, 2022

macOS でテストのビルドを試してみて、うまく行ったので報告します。以下、やってみたビルドとテスト実行の手順です(CMake は不慣れで、かつ Catch2 は全く知らないので、変なところがあればご指摘ください)。

README に手順が書いてありましたね、すみません(汗)README 通りの手順でうまく行くことを確認しました。

@qwerty2501 qwerty2501 force-pushed the feature/append_unit_testing_framework branch from 5d52221 to 69c75b5 Compare May 13, 2022 17:46
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

C++のテスト実装はそこそこの大変さがあることがわかってとても勉強になりました。

CMakeLists.txt Outdated Show resolved Hide resolved
target_include_directories(unit_test PRIVATE ${Catch2_SOURCE_DIR}/src)
target_include_directories(unit_test PRIVATE ${CORE_DIR}/src)
target_link_libraries(unit_test PRIVATE Catch2::Catch2WithMain)
target_link_libraries(unit_test PRIVATE core)
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらでも動作確認できました!

@Hiroshiba Hiroshiba merged commit 6253a8d into VOICEVOX:main May 15, 2022
PickledChair pushed a commit that referenced this pull request Jul 24, 2022
* デフォルト引数はC言語では使えないのでC++のみに有効になるように変更した (#122)

本当はデフォルト引数を消したかったが、使ってる人がいる可能性があるためC++で使ってる場合はそのままにするように修正した

* initializeで全モデルを読み込まなくても良いようにした (#124)

* load_model関数を切り出す

* load_modelとis_model_loadedを足した

* 使われてないエラーメッセージを消去

* Update core/src/core.cpp

Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com>

* Update core/src/core.cpp

Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com>

* Update core/src/core.cpp

Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com>

* pythonのラッパーの型を変更

* load_all_models追加

* return true

Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com>

* workflow_dispatchでもビルド開始可能に (#127)

* ビルド時にREADMEファイルを追加する (#131)

* append unit testing framework (#121)

* Catch2をプロジェクト導入して参考となる1ケースを追加した

* リリース用ビルドにはテストビルドは不要のため無効化する変数を追加した

* テストコードの短縮化を行った

* READMEにテスト実行方法を追加

* テストをビルドするかどうかのフラグをデフォルトOFFにした

* テストをビルドするかどうかのフラグを変更したことによりREADMEを修正

* MSVCに対して足りないオプションを追加

- utf8文字列でコンパイルするように指定
- C++20を明示的に指定
- __cplusplusの値を利用中のC++のバージョンに合わせるように指定(ないとC++98相当になるとか)

* Configがリリースの場合のみに最適化オプションを指定するように修正

* オプションの指定を短くまとめた

* coreの標準ライブラリもバージョンアップした

* compile optionsの修正

* Catch2にもCXX_STANDARD 20を追加

* Windows環境でビルドできるように設定を修正

#121 (comment)

* format 効いてしまっていたところを修正

* static castやnullopt比較をやめ、value関数などを使うようにする (#118)

* use value and has value

* remove has_value

* coreのビルド時にバージョン情報がちゃんと
入るようにする

* hotfix イントネーションの推論をCPUで行うように (#146)

* python example for 0.12, update FFI (#138)

* .gitignore open_jtalk_dic in example (#148)

* .gitignore open_jtalk_dic in example

* modify: example/python/.gitignore

* C++サンプルコードへのリンクを追加 (#142)

* C++サンプルコードへのリンクを追加

* #readme

* Update README.md

* コード署名できるようにする (#164)

* コード署名

* build_util

* artifact/

* a

* remove

* inputのbooleanは文字列として渡ってくるので判定を修正 (#166)

* inputのbooleanは文字列として渡ってくるので判定を修正

* 修正もれ

Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Yuto Ashida <y-chan@y-chan.dev>
Co-authored-by: Yosshi999 <Yosshi999@users.noreply.github.com>
Co-authored-by: nebocco <73807432+nebocco@users.noreply.github.com>
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.

None yet

4 participants