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

Add Pytest and tests for dsu (#5) #8

Merged
merged 11 commits into from
Sep 14, 2020
Merged

Conversation

KATO-Hiro
Copy link
Contributor

WHAT

  • (Add Pytest and tests for dsu (#5) #7)で誤ってtmpブランチからPRを作成してしまったので、masterブランチから改めて作成しました。
  • Pytestを利用したサンプルとしてDSUのテストを書きました。
  • 現状では、テストの雛形を作成することを優先して正常系のみです。

Questions

  • テストの網羅性・ドキュメントの分量について、ご意見をいただけますでしょうか?(このサンプル程度で十分、もう少し増やす・減らすなど)
  • 各関数にあるassertのチェックについても、テストする方針でしょうか?

@KATO-Hiro KATO-Hiro changed the title Add pytest and tests for dsu (#5) Add Pytest and tests for dsu (#5) Sep 12, 2020
Copy link
Owner

@not522 not522 left a comment

Choose a reason for hiding this comment

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

テストは全体的にこのくらいの量でいいと思いますが、以下のテストを追加してください。

  • 何もmergeしていない時点での挙動
  • 既に同じグループになっている要素をmergeした時の挙動
  • 範囲外のindexを指定した時にassertに引っかかること

@not522
Copy link
Owner

not522 commented Sep 12, 2020

.github/workflows/python-package.ymlのコメントアウトを外してpytestを有効化してください。

@KATO-Hiro
Copy link
Contributor Author

ご丁寧なレビューをありがとうございます。

  • 不足しているテストの追加
  • 既存のテストの修正
  • GitHub ActionsでPytestの有効化 などを

修正いたします。

@KATO-Hiro
Copy link
Contributor Author

KATO-Hiro commented Sep 13, 2020

WHAT

  • 下記のテストを追加しました。各メソッドの挙動について、テストしている項目を列挙します。
  • 何もmergeしていない時点での挙動

該当メソッド: test_initial_status()

  • 頂点の同士のペアが、全て同じグループではない
  • 各頂点のサイズは1
  • 各頂点の代表元は、自分自身
  • 各頂点が、別々のグループに属している
  • 既に同じグループになっている要素をmergeした時の挙動

該当メソッド: test_merge_elements_of_same_group()

  • 頂点0と頂点1を2回mergeしました
  • いずれの頂点とも同じグループに属しており、かつ、サイズは2で変わらない
  • 範囲外のindexを指定した時にassertに引っかかること

該当メソッド: test_xxxx_failed_if_invalid_input_is_given()
xxxxは、テストするメソッド名を表す

  • mergesameに関しては、2つの頂点について範囲外のindexを与えました

    • 片方の頂点のみ範囲外(下限もしくは上限を超える場合)となるindexを指定
    • 両方の頂点が範囲外となるindexを指定
  • leadersizeに関しては、1つの頂点について範囲外(下限もしくは上限を超える場合)のindexを与えました

Questions

  • 各メソッドの挙動について、条件の見落とし、もしくは、過剰にチェックしている部分があれば、教えていただけますでしょうか?

@KATO-Hiro
Copy link
Contributor Author

KATO-Hiro commented Sep 13, 2020

WHAT

  • GitHub Actionsで、Pytestを有効化しました
  • ご指摘いただいた不要なファイル(requirements.txt, conftest.py)を削除しました
  • 冗長な記述(テストでのbool値の判定、クラス名のobject)を削除しました
  • test_size()で、sameを使った比較を削除しました
  • test_leader()で、代表元が必ずしも0にはならないことに対応しました

KATO-Hiro and others added 2 commits September 14, 2020 06:31
Reason:
To make sure that leaders of two vertices are same
@not522
Copy link
Owner

not522 commented Sep 14, 2020

ありがとうございます。マージします!

@not522 not522 merged commit 80f0de1 into not522:master Sep 14, 2020
@KATO-Hiro
Copy link
Contributor Author

ありがとうございます。

ご丁寧にレビューしていただき、とても感謝しております。

@KATO-Hiro KATO-Hiro deleted the features/pytest branch October 18, 2020 05:12
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