-
Notifications
You must be signed in to change notification settings - Fork 20
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
test: add PluginSystem.Configure() tests #143
Conversation
4459afe
to
13c1836
Compare
9b146c3
to
dacbf03
Compare
4a05219
to
8e65f30
Compare
5819d20
to
5bcc9fa
Compare
32d689d
to
9d8658f
Compare
9d8658f
to
9b1c784
Compare
} | ||
|
||
p1 := plugin_mock.NewMockPlugin(ctrl) | ||
p1.EXPECT().Setup(gomock.Any()) |
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 a follow-up PR, it would be nice to be explicit about what the Setup takes. I.e. testing that it's getting called with the expected args.
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've updated some of them, but this specific one accepts an array of bytes which is pretty awkward. I think I'd only want to verify the bytes are correct in a test explicitly testing that, to avoid the "data => bytes" logic being repeated everywhere.
pkg/plugin/system/system_test.go
Outdated
var stdout strings.Builder | ||
streams := ioutils.Streams{Stdout: &stdout, Stderr: &stdout} | ||
|
||
pluginProperties := make(map[string]interface { // foo: {}, |
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.
Remove comment?
37366a6
to
33f1f94
Compare
WIP with ugly unformulated code until #135 is merged, but let me know if you have any suggestionsThis could be merged before #135 now. Either way there will be conflicts so doesn't matter which merges first 🤷This PR currently includes #150