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 WorkerPool test #40

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Add WorkerPool test #40

merged 3 commits into from
Oct 11, 2024

Conversation

winebarrel
Copy link
Member

WorkerPoolのテストを追加します。

@winebarrel winebarrel mentioned this pull request Oct 11, 2024
Copy link

@kazukousen kazukousen left a comment

Choose a reason for hiding this comment

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

概ねLGTMですが一点コメントしました 🙏

}
}

func TestWorkerPoolMultiDB(t *testing.T) {
Copy link

@kazukousen kazukousen Oct 11, 2024

Choose a reason for hiding this comment

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

今は正しく動いててよさそうなんですが、手を加えていく際のテストに対する信頼性の観点で以下コメントです↓

これ Worker は引数 Job の Queue フィールドの値をそのままインサートするので、「期待通りのキューと対応するワーカーが動作した」ことを示すテストにはなってないかも、と思いました。具体的には、今の場合、もし実は pool1 しか動作してなかったとしても、気づけなさそうです。

以下のどちらかをテスト設計に組み込むとより良いかもと思いました

  1. WorkerPool ごとに sync.WaitGroup を分ける (意図せず片方の pool が動作していない場合、 wg.Done しないので気付ける)
  2. 各 WorkFunc は引数Jobの Queue をそのまま job_test にインサートしない (意図せず片方の pool が動作していない場合、 job_test の queue の内容が期待と異なるので気付ける)

Copy link
Member Author

@winebarrel winebarrel Oct 11, 2024

Choose a reason for hiding this comment

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

具体的には、今の場合、もし実は pool1 しか動作してなかったとしても、気づけなさそうです。

pool1しか動作していなかった場合、SELECT name, queue FROM job_test の結果が期待する結果と異なるので気付くかなと思ったのですが、いかがでしょうか?

Copy link

@kazukousen kazukousen Oct 11, 2024

Choose a reason for hiding this comment

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

たとえば WorkerPool が que_jobs からジョブを取得する際に queue を無視するバグを仕込んだ場合 (where 句に queue 絞るのを忘れるとかで...) に全部のジョブを pool1 が取得する可能性ができることになり、それを pool1 が queue=queue2 で job_test にインサートするため気付けない... と言ったケースを想定しました。

が、SQLの取得周りは別のレイヤのテストで動作保証するのであればここでは気にすることではないかもしれません...

Copy link
Member Author

Choose a reason for hiding this comment

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

各 WorkFunc は引数Jobの Queue をそのまま job_test にインサートしない (意図せず片方の pool が動作していない場合、 job_test の queue の内容が期待と異なるので気付ける)

あ、こちらは修正しようかと思います

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed f9d6ba3

Copy link

@kazukousen kazukousen left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@winebarrel winebarrel requested a review from kazukousen October 11, 2024 03:23
@winebarrel
Copy link
Member Author

🙏

@winebarrel winebarrel merged commit 2d7a8c4 into v4 Oct 11, 2024
1 check passed
@winebarrel winebarrel deleted the add_workerpool_test_for_v4 branch October 11, 2024 03:23
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