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

C APIのE2Eテストを作る #424

Closed
3 tasks done
qryxip opened this issue Feb 4, 2023 · 12 comments · Fixed by #425
Closed
3 tasks done

C APIのE2Eテストを作る #424

qryxip opened this issue Feb 4, 2023 · 12 comments · Fixed by #425

Comments

@qryxip
Copy link
Member

qryxip commented Feb 4, 2023

内容

#400 (comment)

Pros 良くなる点

  • 単体テストがカバーしない範囲をテストできる
    • ログ出力なども含む

Cons 悪くなる点

  • 実装コスト

実現方法

Rustのintegration test内でDLLのテストをします。

まず/crates/voicevox_core_c_api/tests/runみたいな場所にworkspaceを切り離したbinクレートのパッケージを作り、そこではlibloading (dlopen/LoadLibraryをやるクレート)でDLLを呼び出します。
そしてintegration test (/crates/voicevox_core_c_api/tests/{tts,tts-via-audio-query,get-version,invalid-model,missing-openjtalk-dic,…}.rs)ではそれをビルドし、assert_cmdでテストします。

この方法であれば将来このリポジトリでもcode coverageを取りたいとなったときにこの統合テストもちゃんと対象になる、と思うのですが検証はまだしていません。

VOICEVOXのバージョン

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@qryxip qryxip added the 要議論 実行する前に議論が必要そうなもの label Feb 4, 2023
@Hiroshiba
Copy link
Member

良さそうに感じました!!

openjtalk用の辞書をどう用意し、どう差し込むかが腕の見せ所なのかなと思いました。

@qryxip
Copy link
Member Author

qryxip commented Feb 4, 2023

辞書については今/crates/voicevox_core/src/test_util.rsにあるやつを/crates/test_utilとして独立させ、それを利用することを考えてます。

Rust APIの方のテストも少々すっきりするはず。

-        let open_jtalk_dic_dir = download_open_jtalk_dict_if_no_exists().await;
+        let open_jtalk_dic_dir = test_util::OPEN_JTALK_DIC_DIR;

@qryxip
Copy link
Member Author

qryxip commented Feb 4, 2023

今思い付きましたが、そうするとDLL呼び出しのbinも/crates/test_utilに含められるかも?
よく考えたらworkspaceを切り離す必要性は薄いし。

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 4, 2023

辞書については今/crates/voicevox_core/src/test_util.rsにあるやつを/crates/test_utilとして独立させ、それを利用することを考えてます。

おおおお、素晴らしい便利関数ありましたね、完全に失念していました!!

今思い付きましたが、そうするとDLL呼び出しのbinも/crates/test_utilに含められるかも?
よく考えたらworkspaceを切り離す必要性は薄いし。

workspaceを切り離すというのは、つまりrootに統合テスト用のディレクトリを作ってそこにcargo.tomlを配置する感じでしょうか。(あるいは更に1つディレクトリを用意してその下にtoml配置)
voicevox_core_c_apiをビルドした後の成果物のテストをどこに置くべきか若干難しいですが、たしかにworkspaceを分けずcrates内でやっちゃうのもありなのかなと思いました!

@qryxip
Copy link
Member Author

qryxip commented Feb 5, 2023

workspaceを切り離すというのは、つまりrootに統合テスト用のディレクトリを作ってそこにcargo.tomlを配置する感じでしょうか。(あるいは更に1つディレクトリを用意してその下にtoml配置)

rootというよりは以下のようにtests/下に置いてworkspace.excludeで切り離すことを考えてました。

まず/crates/voicevox_core_c_api/tests/runみたいな場所にworkspaceを切り離したbinクレートのパッケージを作り

(#424 (comment))

ただworkspaceを切り離す必要性はよく考えたら薄いというよりは無い上、workspaceを統合した場合voicevox_core_c_apiから構造体定義を持って来れるというメリットもあります。

voicevox_core_c_apiをビルドした後の成果物のテストをどこに置くべきか若干難しいですが

テスト内で普通にcargo buildしてそれをそのまま使おうと思ってます。実行バイナリにDLLをリンクする必要は無いし、ビルドが終わった後のプロセスからビルドを実行する分には問題無いはずなので。

@qryxip
Copy link
Member Author

qryxip commented Feb 5, 2023

code coverageも試したらちゃんと取れました。

実験
#[no_mangle]
pub extern "C" fn hello() {
    if false {
        println!("this is dead code");
    }
    println!("Hello!");
}
use libloading::Library;

fn main() -> anyhow::Result<()> {
    unsafe {
        let lib = Library::new("./target/debug/liblib.so")?;
        let hello = lib.get::<unsafe extern "C" fn()>(b"hello")?;
        hello();
    }
    Ok(())
}
use duct::cmd;

#[test]
fn test() -> anyhow::Result<()> {
    cmd!("cargo", "build", "-p", "lib", "-p", "run").run()?;
    //assert_cmd::Command::cargo_bin("run")?
    //    .assert()
    //    .success()
    //    .stdout("Hello!\n")
    //    .stderr("");
    Ok(())
}

image

↓ コメントアウトを解除

use duct::cmd;

#[test]
fn test() -> anyhow::Result<()> {
    cmd!("cargo", "build", "-p", "lib", "-p", "run").run()?;
    assert_cmd::Command::cargo_bin("run")?
        .assert()
        .success()
        .stdout("Hello!\n")
        .stderr("");
    Ok(())
}

image

議論することが無ければ実装に入れるとは思いますが、今やるかどうかは優先度次第ですね。
テスト機構はともかくE2Eテスト自体の内容を考えないといけないので、qwertyさんの #370 の後でもよさそうな気がします。

@Hiroshiba
Copy link
Member

ビルド後にテスト、良さそうに感じました!
ただ単体テストするときはそこはテストされない(あるいは省ける)ようになってると開発しやすそう…?

#370 は新しいAPIが増える一方で、とりあえずC APIの挙動は変わらないようにできればと考えてます!
なのでむしろ複合テストがあった方がちゃんと通れてるかテストできて良いかもとちょっと思いました…!

@qryxip
Copy link
Member Author

qryxip commented Feb 5, 2023

ビルド後にテスト、良さそうに感じました! ただ単体テストするときはそこはテストされない(あるいは省ける)ようになってると開発しやすそう…?

重いテスト用に#[ignore]というattributeがあって、それを付ければ「軽いテスト」と分離することができます。実行したいときは--ignored--include-ignoredを付けてcargo testを実行する形になります。

#370 は新しいAPIが増える一方で、とりあえずC APIの挙動は変わらないようにできればと考えてます! なのでむしろ複合テストがあった方がちゃんと通れてるかテストできて良いかもとちょっと思いました…!

なるほど。それなら取り掛かった方がいいですね。

@Hiroshiba
Copy link
Member

#[ignore]、なるほどです。いたれりつくせりだ。

@qryxip qryxip removed the 要議論 実行する前に議論が必要そうなもの label Feb 5, 2023
@qwerty2501
Copy link
Contributor

#370 ではengine互換向けのAPI以外は破壊的変更入ります

@qryxip
Copy link
Member Author

qryxip commented Feb 7, 2023

なるほど。ならcompatible_engineの分だけ作ればいいですかね。

@qwerty2501
Copy link
Contributor

そうですね。現状でも使われてるのはそこなので

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants