Unit-tests for UCNF Close and Request functions and CircleCI go test job #38
Conversation
shanchunyang0919
requested review from
amberimam,
tiswanso,
davetejas,
brecode,
snowbldr,
ziyuguo716,
CosminNechifor,
panyuenlau,
ondrej-fabry,
adodon2go and
yulia2590
There was a problem hiding this comment.
Generally looks like a good start for framing up the UT for this part. I know there's some follow-on subtasks being worked by others so will try to get a bit more details from their work before approving.
CircleCI additional job looks good other than the nit comment I added. Good job finding and using the circleci standard go container.
For the test stuff, the mocks look basically good but as mentioned above I'd like to get a little more details on additional tests being worked.
.circleci/config.yml
Outdated
| working_directory: ~/go/src/github.com/cisco-app-networking/nsm-nse/ | ||
| # runs all the go test recursively | ||
| command: | | ||
| export GOPATH=~/go |
There was a problem hiding this comment.
you wouldn't have to do this if you just change the default GOPATH var setting in defaults-go-tester section
There was a problem hiding this comment.
Hello, Tim. Thank you for leaving this comment. I will make modifications.
| _, err = ucnfe.Close(ctx, conn) | ||
| g.Expect(err).Should(HaveOccurred(), fmt.Sprintf("ERROR : %v", err)) | ||
|
|
||
| } |
There was a problem hiding this comment.
nit: need a newline for compatibility with some tools (e.g. diff/patch)
| } | ||
|
|
||
| // mock implementor | ||
| type MockUcnfBackendReturnNil struct { |
There was a problem hiding this comment.
question: is the "ReturnNil" name suffix a golang mock pattern you've seen or just a name you chose?
There was a problem hiding this comment.
Thank you for asking, this was the name that I chose.
| return nil | ||
| } | ||
|
|
||
| func createUcnfEndpoint() (e *nseconfig.Endpoint) { |
There was a problem hiding this comment.
IMO, this function should be named "createNseConfigEndpoint"
| return | ||
| } | ||
|
|
||
| func createConnection() (conn *connection.Connection) { |
There was a problem hiding this comment.
nit: maybe it's a good idea to start a pattern of naming functions that are primarily to operate on NSM objects as "Nsm" format, e.g. this would be "createNsmConnection"
There was a problem hiding this comment.
Thank you for the advices. I will start working on the these.
| return nil | ||
| } | ||
|
|
||
| func (m *MockUcnfBackendReturnNil) ProcessDPConfig(dpconfig interface{}, update bool) error { |
There was a problem hiding this comment.
It would be much simpler and also more flexible to inject custom behavior (implementation) of these methods by having single mock backed with fields defined as funcs:
type MockUcnfBackendProcessDPConfig struct {
//...
processDPConfig func(dpconfig interface{}, update bool) error
}
//...
func (m *MockUcnfBackendProcessDPConfig) ProcessDPConfig(dpconfig interface{}, update bool) error {
if m.processDPConfig == nil {
return nil
}
return m.processDPConfig(dpconfig, update)
}This way you will get the "..ReturnNil" behavior by default when creating MockUcnfBackendProcessDPConfig and also make it possible to override the implementation by simply setting the field value:
mocked := &MockUcnfBackendProcessDPConfig{
processDPConfig: func(dpconfig interface{}, update bool) error {
return fmt.Errorf("failed to run ProcessDPConfig() method")
},
}This will make it possible to have a single mocked backed struct and much less redundant code.
There was a problem hiding this comment.
Hello Ondrej, thank you very much for the suggestions. I will make modifications.
|
Please create this PR against the wwwin-github.cisco.com repo and we can get folks to give their final approval and merge it. |
PR for NSMNSE-35
link
Added mock functions unit tests for pkg/universal-cnf/config/composite.go::Request(), Close():
In CircleCI config:
CircleCI image for golang: reference
----Updates----
----Updates----(12/11/2020)