-
Notifications
You must be signed in to change notification settings - Fork 2
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
tests: controller integration tests #27
Conversation
5013870
to
e78b382
Compare
Signed-off-by: Guilherme Cassolato <[email protected]>
e78b382
to
35eb125
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment about the waitGroups is the only real concern that I have.
cancel() | ||
} | ||
}() | ||
time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this sleep. Would it not be safer to set up a wait group for the go routines and exit once all have finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goroutine won't finish. It only finishes when the context is done (canceled), hence the wait and the deferred call to cancel()
.
The wait is mainly to give the manager time to start processes such as the metrics service and others. This is more relevant to controller-runtime managed controllers than to informers-based ones. The latter boot up significantly quicker.
cancel() | ||
} | ||
}() | ||
time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again would a wait group be better here.
|
||
.PHONY: test-unit | ||
test-unit: ## Run unit tests. | ||
go test -tags=unit -v ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I don't like using the -v
by default when it is being run in automation, but as there is very limited logs currently I am okay with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Add proper integration tests for the
controller
package.TestStartControllerManaged
test caseAdded target for running the tests:
Other targets introduced:
make test