-
Notifications
You must be signed in to change notification settings - Fork 491
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
Container Setup Test Rewrite With Mocks #9260
Container Setup Test Rewrite With Mocks #9260
Conversation
…as in tests) can be used for instantiation.
…d mocks for both APICaller and FacadeCaller.
…on JujuConnSuite.
Replaces test for container manager config acquisition error.
…ld be OK to run on Windows.
@@ -16,7 +16,7 @@ import ( | |||
|
|||
// NewContainerManager creates the appropriate container.Manager for the | |||
// specified container type. | |||
func NewContainerManager(forType instance.ContainerType, conf container.ManagerConfig) (container.Manager, error) { | |||
var NewContainerManager = func(forType instance.ContainerType, conf container.ManagerConfig) (container.Manager, error) { |
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.
I'm still not a huge fan of this, I'd rather we didn't patch these at test time, but considering the changes required to make this a dependency I'm all for this atm.
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.
Agreed. I don't like calls to patch at all.
If we lived in an ideal world, all these factory-type dependencies would be injected at the top, through the manifold by the dependency engine. Then at worker creation you could pass in a mock by its indirection.
Rolling it all back up would be a hell of a job though.
Check job looks like an intermittent failure. Let's try a |
Description of change
The ContainerSetupSuite tests previously used JujuConnSuite and were racy.
This patch rectifies this with the following changes:
Note for Reviewers: worker/provisioner/container_initialisation_test.go bears little resemblance to its old form. It might be better than to read this in its own right rather than the diff.
QA steps
The re-written tests pass consistently.
Documentation changes
None.
Bug reference
N/A