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

テストの追加 #30

Merged
merged 17 commits into from
Aug 3, 2022
Merged

テストの追加 #30

merged 17 commits into from
Aug 3, 2022

Conversation

dc7290
Copy link
Member

@dc7290 dc7290 commented Aug 1, 2022

概要

既存のコードに関してテストを追加しました。

  • tests/utils/isCheckValue.test.ts

基本的な値に対してisXXX系関数のテストをしました。

  • tests/utils/parseQuery.test.ts

fiedsに配列、数字、文字列を指定した場合とオブジェクトではない値を入力した際のテストをしました。

  • tests/createClient.test.ts

意図通りの関数が返されるかと、未指定の引数があったときのエラーをテストしました。

  • tests/get.test.ts

get関数を使って、
リスト形式、リスト形式の詳細、オブジェクト形式それぞれのアクセスでデータが取得できるかと、
未指定の引数があった時、サーバエラー時の処理をテストしました。

構成

テストツール: Jest
モックサーバー: MSW

@dc7290 dc7290 self-assigned this Aug 2, 2022
@dc7290 dc7290 requested a review from hiro08gh August 2, 2022 07:47
@dc7290 dc7290 marked this pull request as ready for review August 2, 2022 07:47
型定義のところにもun-defルールとかが適用されてしまうのを修正
import { createClient } from '../src/createClient';

describe('createClient', () => {
// Normal system
Copy link
Contributor

Choose a reason for hiding this comment

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

test内容で正常系と異常系がわかるので、コメントは不要かと思ったのですがどうでしょうか?

Copy link
Member Author

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.

get.test.tsファイルに関してはそのままでよさそうでしょうか?

Copy link
Contributor

Choose a reason for hiding this comment

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

前回参加したリーダブルテストコードの資料に結構感化されています・・・!テストはドキュメントだとすると、コメントよりテストケースでちゃんと意図を伝えることが正しい方向性かなと思いました!!自分でもまだテストコードに対して明確なベストプラクティスは模索中ですが・・・!
https://speakerdeck.com/jnchito/number-vstat?slide=40

でぃーすけさんから見て納得感があれば、get.test.tsも変更お願いします!

Copy link
Member Author

Choose a reason for hiding this comment

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

めっちゃわかります!!
この資料僕も読んでいまして、おっしゃる通りで「テストはドキュメント」っていう基準がかなり納得感あるんですよね!
(イベント自体を見れなかったのが、本当に心苦しい・・・)

それでいくと、自分も get.test.ts も変更した方がいいと思ったので、こちらも変更いたします!!

expect(typeof client.getObject === 'function').toBe(true);
});

// Abnormal system
Copy link
Contributor

Choose a reason for hiding this comment

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

同上です!

@dc7290
Copy link
Member Author

dc7290 commented Aug 3, 2022

@hiro08gh
再度レビューお願いいたします!!

@hiro08gh
Copy link
Contributor

hiro08gh commented Aug 3, 2022

ありがとうございます!LGTMです!Squash and mergeでお願いします!

@dc7290 dc7290 merged commit 7c00816 into main Aug 3, 2022
@dc7290 dc7290 deleted the add-test branch August 3, 2022 03:01
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