Skip to content
This repository was archived by the owner on Sep 27, 2023. It is now read-only.

Add api v3 client#40

Merged
shoe116 merged 4 commits intodevelopmentfrom
add-api-v3-client
Aug 27, 2021
Merged

Add api v3 client#40
shoe116 merged 4 commits intodevelopmentfrom
add-api-v3-client

Conversation

@akiomik
Copy link
Copy Markdown
Contributor

@akiomik akiomik commented Aug 25, 2021

No description provided.

@akiomik akiomik self-assigned this Aug 25, 2021
@akiomik akiomik added the enhancement New feature or request label Aug 25, 2021
@akiomik akiomik requested a review from shoe116 August 25, 2021 06:01
Copy link
Copy Markdown
Contributor

@shoe116 shoe116 left a comment

Choose a reason for hiding this comment

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

良さそうです

Comment thread src/api/v3/client.test.ts
@@ -0,0 +1,166 @@
import * as company from '~/__mocks__/fixtures/v3/company'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ここ、好みもあるけど test 用の api token 使って実際叩くテスト書くのもありっちゃありな気がする。
レスポンスと細かい部分というよりは、http status だけチェックするみたいな

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

個人的にはあんまり好きじゃないです。「外部サービスに何を期待しているか」が曖昧になるし、外部に依存してると意図してない形でテストが落ちたりするので、単体テスト (googleのいうsmall test) はモックを使って書いた方がいいと思ってます。単体テストとは別にモックせずにテストするテストケースがあるのはいいと思うけど、developのCIとかのレベルで頻繁に回すようなものではないかなとも思うので実行タイミングが難しい…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

了解 確かにここにはないほうが自然だよね。
CI がいい感じに別の stage でやってくれるとかっこいい感じ

@shoe116 shoe116 merged commit 7bd183c into development Aug 27, 2021
@akiomik akiomik mentioned this pull request Nov 6, 2021
@akiomik akiomik deleted the add-api-v3-client branch November 10, 2021 07:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants