-
Notifications
You must be signed in to change notification settings - Fork 149
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
Integration test suite #317
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Andrea Panattoni <[email protected]>
Signed-off-by: Andrea Panattoni <[email protected]>
Signed-off-by: Andrea Panattoni <[email protected]>
f, err := os.OpenFile(p.methodCallsRecordingFilePath, | ||
os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | ||
if err != nil { | ||
log.Println(err) |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning test
call to OpenFile
819bcfa
to
423fe1c
Compare
Pull Request Test Coverage Report for Build 13185028153Details
💛 - Coveralls |
d8f433c
to
7c1f069
Compare
c452d90
to
67498c8
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.
This is great!
I left two small comments
test/integration/test_sriov_cni.sh
Outdated
|
||
make_container "container_1" | ||
|
||
export CNI_NETNS=/run/netns/container_1_netns |
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.
is it possible to move this export inside the make_container or it doesn't work like that in bash? :P
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.
surprisingly, it works! and it make the code cleaner. fixing
export CNI_COMMAND=ADD | ||
assert invoke_sriov_cni | ||
assert_file_contains "${DEFAULT_CNI_DIR}/enp175s0f1.calls" "LinkSetVfVlanQosProto enp175s0f1 0 1234 0 33024" | ||
|
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 think we need to remove this file after every test no?
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.
it's not mandatory, because the setup()
function creates a different DEFAULT_CNI_DIR at every test:
setup() {
...
DEFAULT_CNI_DIR=$(mktemp -d)
export DEFAULT_CNI_DIR
}
But it is still a good practice. Adding a teardown()
function
67498c8
to
411069b
Compare
Having a test suite that covers the code from an out-of-process perspective helps in testing specific scenarios, e.g. when the concurrency of multiple program instances occurs. Also, using test doubles instead of real SR-IOV capable NICs makes tests fast and portable, in a way that can be run at every commit. Add a test suite that invokes a system-mocked version of the sriov CNI binary, in which the `/sys` filesystem content is defined at code, and the network interfaces are of type dummy. Add `test-integration` make target to invoke the test suite. Signed-off-by: Andrea Panattoni <[email protected]>
Signed-off-by: Andrea Panattoni <[email protected]>
411069b
to
802de56
Compare
Having a test suite that covers the code from an out-of-process perspective helps
in testing specific scenarios, e.g. when the concurrency of multiple program instances
occurs. Also, using test doubles instead of real SR-IOV capable NICs makes tests fast
and portable, in a way that can be run at every commit.
Add a test suite that invokes a system-mocked version of the sriov CNI binary, in which
the
/sys
filesystem content is defined at code, and the network interfaces are of type dummy.Add
test-integration
make target to invoke the test suiteMove cni commands to pkg/cnicommands package
Allow injecting NetlinkManager in SriovManager
refer to the README.md file for the test architecture