Skip to content

Conversation

@Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Dec 28, 2024

内容

VVPPデフォルトエンジンを使うテストを書きます。

途中でelectronのダイアログが来ますが、ダイアログ表示関数を直接上書きすることで無理やり突破します。
将来的にはelectronのダイアログではなくブラウザ表示になる予定なのでこのワークアラウンド不要になる予定です。

変更行数は少ないですが、結構複雑度が上がってしまっています。解決策募集中。。

  • VVPPデフォルトエンジン用の.envが増えた
    • VVPPデフォルトエンジンに統一されたらまた減るはず
  • デフォルトエンジン情報はimport.meta.env経路以外にprocess.envルートもできた
    • テストしたい環境が2つになったため
    • これもVVPPデフォルトエンジンに統一されたらもとに戻るはず
  • electronダイアログを無理やり通るためのモックが増えた
    • これはそもそもGUIが整備されたらダイアログ自体がなくなるはず

関連 Issue

その他

おそらく動くのですが、VOICEVOXエンジンをダウンロードする部分が重くてタイムアウトになると思います。
Nemo用のlatestDefaultEngineInfosを作ろうと思います。

@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Dec 28, 2024

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:fc10207

Comment on lines 39 to 42
process.env.VITE_7Z_BIN_NAME =
(options.mode === "development"
(mode !== "production"
? path.join(import.meta.dirname, "vendored", "7z") + path.sep
: "") + sevenZipBinName;
Copy link
Member Author

Choose a reason for hiding this comment

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

testのときも7zのパスの注入が働くように変更しました。

@Hiroshiba
Copy link
Member Author

テストが通ったのでレビュー開けます!!

@Hiroshiba Hiroshiba marked this pull request as ready for review December 28, 2024 21:47
@Hiroshiba Hiroshiba requested a review from a team as a code owner December 28, 2024 21:47
@Hiroshiba Hiroshiba changed the title [WIP]: VVPPデフォルトエンジンを使うテストを書く test: VVPPデフォルトエンジンを使うテストを書く Dec 28, 2024
Comment on lines +53 to -57
log.error(`Failed to install ${vvppPath},`, e);
dialog.showErrorBox(
"インストールエラー",
`${vvppPath} をインストールできませんでした。`,
);
log.error(`Failed to install ${vvppPath},`, e);
Copy link
Member Author

@Hiroshiba Hiroshiba Dec 28, 2024

Choose a reason for hiding this comment

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

エラーになったあとダイアログで止まってしまってログが出ておらず、原因にたどり着くのに少し時間がかかった。
エラー直後は最初にログを吐かないといけなそう。

Copy link
Member Author

@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.

@sevenc-nanashi
変更行数の割にかなり設計が複雑なのでヘビーかもなのですが、レビューお願いしても良いでしょうか 🙇 🙇 🙇

approveの自信がなかったらとりあえずコメントだけとかでも大丈夫です!
このPRの実装に若干自信がないので誰かの目が入ってほしい気持ち。

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

LGTMです。

/** .envからデフォルトエンジン情報を読み込む */
export function loadEnvEngineInfos(): EnvEngineInfoType[] {
// electronのときはプロセスの環境変数を参照する。
// NOTE: electronテスト環境を切り替えるため。テスト環境が1本化されればimport.meta.envを使う。
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
Member Author

Choose a reason for hiding this comment

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

おっと 僕の脳内ストーリーだとこんな感じです!

  • 製品版の起動がVVPPデフォルトエンジンに1本化される
  • 必要なテストが1種類になる
  • process.env.VITE_DEFAULT_ENGINE_INFOSが不要になる
  • ここのNOTEコメントが不要になる!

もし手元の環境でVITE_DEFAULT_ENGINE_INFOSを変えたい場合もimport.meta.env側を使えば良いはずなので、普通にimport.meta.envに共通化できる・・・・・はず・・・?

Copy link
Member Author

@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.

レビューありがとうございます!!
ちなみにもしかしたら割とサクッとレビューできたりしましたか・・・? 👀
(このPRの変更は相当文脈が深く、 @sevenc-nanashi さんも把握できてないポイントがいくつかありそうで結構難しいお願いをしてしまったかなと思っています)

@Hiroshiba Hiroshiba merged commit 7cc32cc into VOICEVOX:main Dec 29, 2024
10 checks passed
@Hiroshiba Hiroshiba deleted the VVPPデフォルトエンジンを使うテストを書く branch December 29, 2024 22:50
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