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

tests cleanup #9

Merged
merged 8 commits into from
Feb 8, 2021
Merged

tests cleanup #9

merged 8 commits into from
Feb 8, 2021

Conversation

kaworu
Copy link
Member

@kaworu kaworu commented Feb 5, 2021

Having many tests in one big function is not ideal. This PR split some tests into their own test function along with some other cleanup. Probably best viewed commit by commit.

@kaworu kaworu requested review from tklauser and rolinh February 5, 2021 10:20
@kaworu kaworu self-assigned this Feb 5, 2021
@kaworu kaworu force-pushed the pr/kaworu/test-cleanup branch from ac6ee7b to b7a377f Compare February 5, 2021 10:32
tklauser
tklauser reviewed Feb 5, 2021
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Once godoc remark and a few comment spelling nits. Otherwise LGTM.

task.go Outdated Show resolved Hide resolved
workerpool_test.go Outdated Show resolved Hide resolved
workerpool_test.go Outdated Show resolved Hide resolved
workerpool_test.go Outdated Show resolved Hide resolved
@tklauser tklauser force-pushed the pr/kaworu/test-cleanup branch from 41e22ae to b7a377f Compare February 5, 2021 15:05
@tklauser
Copy link
Member

tklauser commented Feb 5, 2021 •
edited
Loading

@kaworu sorry, I accidentially pushed two commits to this branch because I wanted to base one of my own branch for #10 on this one. The commits should now be gone again.

@tklauser tklauser mentioned this pull request Feb 5, 2021
Merged
rolinh
rolinh requested changes Feb 5, 2021
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm overall besides what @tklauser already pointed out and some clarification needed for a test comment.

workerpool_test.go Outdated Show resolved Hide resolved
kaworu added 5 commits February 8, 2021 10:30
@kaworu
task: comments
70cb8b6 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: remove unneeded WorkerPool capacity check
e30bab3 
This is already tested by TestWorkerPoolCap.

Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: extract the tasks channel capacity check in its own test
efc75f3 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: move TestWorkerPoolCap
08c0507 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: add TestWorkerPoolConcurrentTasksCount
b05f661 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu kaworu force-pushed the pr/kaworu/test-cleanup branch from b7a377f to 04b98b6 Compare February 8, 2021 09:37
kaworu added 3 commits February 8, 2021 10:37
@kaworu
tests: ensure the exact error expected in TestWorkerPoolManyClose
cdf684f 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: extract submit-after-close in its own test
e8d3fbc 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu
tests: extract drain-after-close in its own test
04b98b6 
Signed-off-by: Alexandre Perrin <alex@kaworu.ch>
@kaworu kaworu merged commit e2ba866 into master Feb 8, 2021
@kaworu kaworu deleted the pr/kaworu/test-cleanup branch February 8, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser left review comments

@rolinh rolinh rolinh approved these changes

Assignees

@kaworu kaworu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaworu @tklauser @rolinh