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 unit tests for package safe #1517

Merged
merged 1 commit into from May 1, 2017
Merged

Add unit tests for package safe #1517

merged 1 commit into from May 1, 2017

Conversation

gottwald
Copy link
Contributor

This PR adds tests to package safe.

In some cases the package is a bit awkward to test.
I also noticed something strange in the code here: https://github.com/containous/traefik/blob/e2fdc27d642c9436c9b925e956290e7df95c43bf/safe/routine.go#L110-L117

Pool.Start() starts the Pool.routines, but the only way to add a routine (not routineCtx) from outside of the package is by calling Pool.Go(goroutine), which already starts the goroutine.

The code was added with this commit when routineCtx got introduced: bea5ad3#diff-f7272237e30bd2c1dc9ccb7f7b2ce583R89
Maybe an issue should be opened to find out if Pool.routines started by Pool.Start() are used at all.

Due to the tests I also found a bug in the Pool.Start() implementation, where a copy of a struct was modified instead of the struct itself.

@ldez
Copy link
Member

ldez commented Apr 30, 2017

Awesome job! The tests are gold ❤️

@ldez ldez added kind/enhancement a new or improved feature. priority/P1 need to be fixed in next release labels Apr 30, 2017
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ldez ldez removed the priority/P1 need to be fixed in next release label Apr 30, 2017
@ldez ldez added this to the 1.3 milestone Apr 30, 2017
@emilevauge
Copy link
Member

@gottwald

I also noticed something strange in the code here: https://github.com/containous/traefik/blob/e2fdc27d642c9436c9b925e956290e7df95c43bf/safe/routine.go#L110-L117

Pool.Start() starts the Pool.routines, but the only way to add a routine (not routineCtx) from outside of the package is by calling Pool.Go(goroutine), which already starts the goroutine.

The reason is to relaunch all the routines after a call to Stop. This is used in cluster context where we have to launch some specific go routines in a master instance.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Awesome work @gottwald
LGTM

Also fix a bug in the code found due to the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants