-
Notifications
You must be signed in to change notification settings - Fork 95
Generate an instrumented vDVS plugin binary #1633
Generate an instrumented vDVS plugin binary #1633
Conversation
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.
Overall looks good, and I have a few comments.
@@ -16,6 +16,8 @@ | |||
// VMDK Docker driver sanity tests. |
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.
Could you add some comments about what is the sanity test is doing?
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 test was left out with the tags .. so adding a tag for the test to control during the build time.
client_plugin/Makefile
Outdated
|
||
$(SHARED_PLUGIN_BIN): $(COMMON_SRC) $(SHARED_PLUGIN_SRC) | ||
@-mkdir -p $(BIN) && chmod a+w $(BIN) | ||
$(GO) build --ldflags '-extldflags "-static"' -o $(SHARED_PLUGIN_BIN) $(PLUGIN)/shared_plugin | ||
|
||
$(BIN)/$(INSTRUMENTED_PLUGIN_BIN): $(COMMON_SRC) $(VMDKOPS_MODULE_SRC) $(E2E_TEST_SRC) ./vmdk_plugin/*_test.go |
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.
Please define "VMDK_PLUGIN_TEST_SRC"=./vmdk_plugin/*_test.go" here and use $(VMDK_PLUGIN_TEST_SRC)".
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.
done!
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.
LGTM with a couple of minor comments
client_plugin/Makefile
Outdated
@@ -72,7 +73,7 @@ PLUGIN_BIN = $(BIN)/$(PLUGNAME) | |||
SHARED_PLUGIN_BIN = $(BIN)/$(SHARED_PLUGNAME) | |||
|
|||
# all binaries for VMs - plugin and tests | |||
VM_BINS = $(PLUGIN_BIN) $(BIN)/$(VMDKOPS_TEST_MODULE).test $(BIN)/$(PLUGNAME).test | |||
VM_BINS = $(PLUGIN_BIN) $(BIN)/$(VMDKOPS_TEST_MODULE).test $(BIN)/$(PLUGNAME).test $(BIN)/$(INSTRUMENTED_PLUGIN_BIN) |
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.
pls add a comment about "instrumented for <what is it instrumented for?"
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.
done!
client_plugin/Makefile
Outdated
@@ -105,6 +106,12 @@ SHARED_PLUGIN_SRC = shared_plugin/main.go drivers/shared/shared_driver.go | |||
|
|||
TEST_SRC = ../tests/utils/inputparams/testparams.go | |||
|
|||
E2E_TEST_SRC = ../tests/utils/admincli/*.go ../tests/utils/dockercli/*.go \ |
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.
should it be ../tests/{utils,constants}//.go ../test/e2e/*.go ?
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.
Well it is not needed, I have removed.
Thanks for pointing out!
94c1b77
to
1cb7d4d
Compare
@shuklanirdesh82 with this instrumented binary will we be able to use -check.v flag when run tests (as go test -check.v) so that verbose details of which test passed and failed inside a test suite are printed? |
No @pshahzeb why do you think so? |
As per the offline discussion we had had over the issue of #1509, I was wondering if it would be possible to do so. |
Fixes #1589
Through this PR, we are able to generate instrumented vDVS binary and will be placed at
build
folder.Testing done: Tested locally
Following steps were performed to test out generated binary
build/vdvs-instrumented
to docker host./vdvs-instrumented -test.coverprofile=e2e.cov &
make test-e2e -test.run "*Basic*"
(locally changed the tag to pick up basic test only)ctrl+c
to end the program (on the docker host where step#2 is performed)e2e.cov
is generated into the current directorylater
e2e.cov
would be passed togo tool cover
to analyze further.Note:
e2e.cov
is renamed ase2e.txt
to upload here. e2e.txt