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

make_docs.pySettingLoaderをMock化したい #751

Closed
aoirint opened this issue Oct 1, 2023 · 3 comments
Closed

make_docs.pySettingLoaderをMock化したい #751

aoirint opened this issue Oct 1, 2023 · 3 comments
Labels

Comments

@aoirint
Copy link
Member

aoirint commented Oct 1, 2023

内容

#711 で提案した内容です。

APIドキュメントを生成するmake_docs.pyでは、実際に設定ファイルをファイルシステムから読み込む機能を持ったSettingLoaderをインスタンス化していますが、ドキュメントの生成には実際の機能は不要なため、ダミーの設定を返すMockに置き換えたいです。

Pros 良くなる点

  • make_docs.pyの機能が単純になる
  • generate_appを再利用しやすくなる

Cons 悪くなる点

  • コード量が増える

実現方法

  • SettingLoaderのインタフェースを定義する抽象クラスSettingLoaderBaseを作成する
  • SettingLoaderSettingLoaderBaseを継承するようにする
  • SettingLoaderBaseを継承するMockSettingLoaderを作成する
  • generate_appの引数setting_loaderの型をSettingLoaderBaseに変更する
  • make_docs.pygenerate_app呼び出しでSettingLoaderの代わりにMockSettingLoaderをインスタンス化するようにする

VOICEVOXのバージョン

0.15 開発版

その他

関連Issue

@aoirint aoirint added 機能向上 優先度:低 (運用中止) labels Oct 1, 2023
@github-actions github-actions bot added OS 依存:linux Linux に依存した現象 OS 依存:mac macOS に依存した現象 OS 依存:win Windows に依存した現象 labels Oct 1, 2023
@aoirint aoirint removed OS 依存:mac macOS に依存した現象 OS 依存:linux Linux に依存した現象 OS 依存:win Windows に依存した現象 labels Oct 1, 2023
@Hiroshiba
Copy link
Member

issue作成ありがとうございます!

SynthesisEngineとSynthesisEngineBaseみたいに、ということですよね! すごく良いと思います!!

@tarepan
Copy link
Contributor

tarepan commented Feb 20, 2024

Mock化の必要性が低減した、と考えています。

Mock化の利点は次の2点かと思います:

A. 処理スキップによる高速化
B. 設定ファイルへの配慮不要化による容易化

#855 により、SettingLoader は設定ファイル無しで機能するようになりました。
よって SettingLoader は設定ファイルへの配慮無しに make_docs.py 等で利用可能、つまり B の利点が無くなりました。
Aの速度は実務上ほぼ無視できるため、Mock化の利点はほぼ無くなりました。
Interface導入は想像以上にメンテコストが掛かるため、利点無しで導入するにはデメリットが大きいです。

よって、状況の変化により、Mock化の必要性は無くなったと考えます(issue close可能)。

@Hiroshiba
Copy link
Member

なるほどです!

他にも、まあ将来設定値を変えた場合のrun.py全体のテストとかをしたくなったら設定のmockが必要になるかもなのですが、その場合はどちらかというとSettingLoaderをmockにするよりSetting構造体をDI可能にする形を取る気がします。

他にはちゃんと設定が保存されるかなどのテストにもmockは便利そうですが、現状設定ファイルパスがDI可能になっているため、tmpファイルのパスを指定すればそのテストもあまり不自由なく可能そうです。

ということでmock化は必然ではなくなったと判断し、closeさせていただきます!

@Hiroshiba Hiroshiba closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants