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.

Sign up for GitHub

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

Jump to bottom

Add: vitestでテストが動くように #1074

Merged
merged 21 commits into from
Feb 25, 2023
Merged

Add: vitestでテストが動くように #1074

merged 21 commits into from
Feb 25, 2023

Conversation

sevenc-nanashi
Copy link
Member

内容

LibraryPolicy.vueとHowToUse.vueのテストを追加します。

関連 Issue

スクリーンショット・動画など

（なし）

その他

単体テストを書くのが初めてなので、色々とやっちゃ行けないことをしているかもしれません。

@sevenc-nanashi sevenc-nanashi marked this pull request as draft December 29, 2022 12:12
@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review January 18, 2023 14:40
@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner January 18, 2023 14:40
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team January 18, 2023 14:40
@sevenc-nanashi
Copy link
Member Author

CIが通るように出来ました

@sevenc-nanashi
Copy link
Member Author

Vitest化できました！

Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
tests/unit/engines.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
tests/unit/engines.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
tests/unit/utils.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
tests/unit/dummyImage.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
_typos.toml Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Feb 25, 2023
View reviewed changes
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.

LibraryPolicy.spec.tsがVuexのstoreをゼロから書く形になっていますが、LibraryPolicyをVuex日依存の形に書き換えるのがやはり良いのかなと思いました！
（関数名などのシグネチャのコピペが大量発生するので、テストが増えれば増えるほど、Vuex側を書き換えづらくなってしまうためです）

提案ですが、このPRはLibraryPolicyの追加と、vitestを通るようにする２つの機能があると思います。
LibraryPolicyの追加は別PRにするというというのはどうでしょう？

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Feb 25, 2023 •
edited
Loading

LibraryPolicyの追加は別PRにするというというのはどうでしょう？

ですね、そのほうが色々やりやすい感じがします。
ダミーのエンジンやwrapQPageはどうしましょう？残しますか？

@Hiroshiba
Copy link
Member

ダミーのエンジンやwrapQPage

やっぱり必要になったときに追加、が綺麗だと思います！
なのでいったん消去だと嬉しいかもです。
（面倒であれば残していただいても、まあ）

@sevenc-nanashi
Copy link
Member Author

このPRはHowToUse.spec.tsだけにします。
LibraryPolicyの構造変更＆テストは別のPRでやることにします。

@sevenc-nanashi sevenc-nanashi changed the title Add: LibraryPolicyのテストを追加 Add: vitestでテストが動くように Feb 25, 2023
Hiroshiba
Hiroshiba approved these changes Feb 25, 2023
View reviewed changes
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！！

問題ないと思うのでマージします！！

@Hiroshiba Hiroshiba merged commit 2c4d7a6 into VOICEVOX:main Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sevenc-nanashi @Hiroshiba