Skip to content

Conversation

@Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Jan 4, 2025

内容

エンジンモックを使った音声書き出しのテストを書いてみました。
実際の音声も手元にpullしたりダウンロードしたりすれば聞けます。

その他

ブラウザ内でのファイル書き込みをどうモックするかで悩みました。
最終的にbackendのwriteFileをDI(というよりもmock?)することで解決しました。

他の方法としてブラウザに標準的についてるFileHandleあたりを差し替えることもできるんですが、↓の用にいろいろ問題があったのでやめました。

  • PlaywrightはshowOpenFilePickerなどのファイル選択ダイアログをmockできない
  • showOpenFilePickerをDIして適当なFileHandlerを返すこともできるけど、FileHandler周りのテストをPlaywrightがサポートしていない
  • ブラウザ版backendのFileHandler周りの実装に直で依存してしまう
    • まだブラウザ版のファイル管理アーキテクチャが成熟しきったあとなら依存しても良さそう

あとテストをplaywrightではなくvitestのブラウザモードで実装するのも考えましたがやめました。
試したところvitest-browser-vueがまだ色々と対応していないのか、ファイルが読み込めてなさそうなエラーが出たので引き返しました。もうちょっと様子見になりそう。

0:36:48 [vite] Internal server error: At least one <template> or <script> is required in a single file component. C:/Users/hihok/Github/voicevox/src/components/App.vue

@Hiroshiba Hiroshiba requested a review from a team as a code owner January 4, 2025 15:24
@Hiroshiba Hiroshiba marked this pull request as draft January 4, 2025 15:25
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Jan 18, 2025

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

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

@Hiroshiba Hiroshiba marked this pull request as ready for review March 1, 2025 15:41
@Hiroshiba Hiroshiba changed the title [wip] test: 音声書き出しe2eテスト test: 音声書き出しe2eテスト Mar 1, 2025
@Hiroshiba Hiroshiba requested a review from sevenc-nanashi March 1, 2025 15:42
Comment on lines -29 to +30
interrogativeUpspeak: false,
adjustPauseLength: true,
interrogativeUpspeak: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

ポーズの長さを変えるのもテストしたかったのでモックを修正しました。

text: "、",
vowel: "pau",
vowelLength: 1 - 1 / (accentPhrases.length + 1),
vowelLength: 1 - 1 / (accentPhrases.length + 2),
Copy link
Member Author

Choose a reason for hiding this comment

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

今のaccentPhrasesの数がaccentPhrases.lengthに入っていて、最初は0なので1 - 1/1になって0(ポーズの長さが0)になってしまってたので、+2にしてみました。

Copy link
Member Author

Choose a reason for hiding this comment

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

window.backends関数のモックを作るにあたっていくつか妥協したポイントもあったのでコメント。
より良い解決策があれば試したいかも。

  • mockを戻せるように作ってない
    • 今モック化したのはダイアログ表示の部分と、ファイル書き出しの部分
    • ダイアログは表示した瞬間ハングするのと、ファイル書き出しもできないので、まあどちらも戻さなくても問題にならないかなと思い、とりあえずそのままにしています
    • 戻せるようにも作れそうだけど、汎用的に書くのは結構難しいかも(全部のモックを戻す関数を作るとかが難しそう)
  • 一般化していない
    • 他にモックを刺したかった時にさくっとモックをさせるように作れてない
    • 今のうちに共通化した方がいいかもだけど・・・良い方法がパッと思いつかなかったので一旦そのままにしています

@Hiroshiba
Copy link
Member Author

@sevenc-nanashi 完成したのでレビューお願いできると・・・! 🙇

妥協したポイントこちらです。。

なんかもっとこうした方がいいんじゃないかとかあれば。。。
(そもそもこんだけ複雑なんだったらテストいらないのでは、みたいな説もありそう。。)

@Hiroshiba Hiroshiba requested review from Copilot and sevenc-nanashi and removed request for sevenc-nanashi April 12, 2025 10:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • tests/unit/mock/engineMock/snapshots/index.spec.ts.snap: Language not supported
Comments suppressed due to low confidence (2)

src/mock/engineMock/index.ts:104

  • Using a null value for pauseLength may lead to type or runtime issues if downstream logic expects a numeric value. Consider initializing this parameter with a default numeric value or handling null explicitly.
pauseLength: null,

src/mock/engineMock/talkModelMock.ts:181

  • [nitpick] The change in the divisor from (accentPhrases.length + 1) to (accentPhrases.length + 2) alters the computed vowelLength, potentially affecting audio output. Verify that this adjustment meets the intended voice synthesis requirements.
vowelLength: 1 - 1 / (accentPhrases.length + 2),

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.

細かいところを書きましたが、それ以外は問題ないと思います。

@Hiroshiba Hiroshiba requested a review from Copilot April 12, 2025 13:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • tests/unit/mock/engineMock/snapshots/index.spec.ts.snap: Language not supported
Comments suppressed due to low confidence (2)

src/mock/engineMock/talkModelMock.ts:181

  • The denominator in the vowelLength calculation changed from (accentPhrases.length + 1) to (accentPhrases.length + 2). Please verify that this adjustment is intentional and correctly reflects the desired audio timing behavior.
vowelLength: 1 - 1 / (accentPhrases.length + 2),

src/mock/engineMock/index.ts:104

  • Using null for pauseLength may lead to unexpected behavior if a numeric value is expected. Confirm that this value is acceptable by the engine/mock configuration.
pauseLength: null,

@Hiroshiba
Copy link
Member Author

たぶん大丈夫だと思うのでマージします! レビューありがとうございました!!

@Hiroshiba Hiroshiba enabled auto-merge April 12, 2025 13:14
@Hiroshiba Hiroshiba added this pull request to the merge queue Apr 12, 2025
Merged via the queue into VOICEVOX:main with commit a2f398a Apr 12, 2025
11 checks passed
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.

2 participants