Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Generate an instrumented vDVS plugin binary #1633

Merged
merged 2 commits into from
Jul 29, 2017
Merged

Generate an instrumented vDVS plugin binary #1633

merged 2 commits into from
Jul 29, 2017

Conversation

shuklanirdesh82
Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 commented Jul 25, 2017 •
edited
Loading

Fixes #1589

Through this PR, we are able to generate instrumented vDVS binary and will be placed at build folder.

Testing done: Tested locally

make clean build-all
go tool vet .
gofmt -s -l -w .
go build --ldflags '-extldflags "-static"' -o ../build/docker-volume-vsphere github.com/vmware/docker-volume-vsphere/client_plugin/vmdk_plugin
go test -c -o ../build/vmdkops.test github.com/vmware/docker-volume-vsphere/client_plugin/drivers/vmdk/vmdkops -cover
go test -c -o ../build/docker-volume-vsphere.test github.com/vmware/docker-volume-vsphere/client_plugin/vmdk_plugin -cover -tags sanity
go test -coverprofile=/tmp/cover.out -coverpkg=github.com/vmware/docker-volume-vsphere/client_plugin/... -c -o ../build/vdvs-instrumented github.com/vmware/docker-volume-vsphere/client_plugin/vmdk_plugin -tags r -covermode count
warning: no packages being tested depend on github.com/vmware/docker-volume-vsphere/client_plugin/drivers/shared
warning: no packages being tested depend on github.com/vmware/docker-volume-vsphere/client_plugin/shared_plugin
make  --directory=../esx_service build

Following steps were performed to test out generated binary

  1. make clean build-all
  2. copying build/vdvs-instrumented to docker host
  3. run instrumented binary instead of installing plugin
    ./vdvs-instrumented -test.coverprofile=e2e.cov &
  4. Run some basic test make test-e2e -test.run "*Basic*" (locally changed the tag to pick up basic test only)
  5. hit ctrl+c to end the program (on the docker host where step#2 is performed)
  6. look for e2e.cov is generated into the current directory

later e2e.cov would be passed to go tool cover to analyze further.

Note: e2e.cov is renamed as e2e.txt to upload here. e2e.txt

lipingxue
lipingxue suggested changes Jul 26, 2017
View reviewed changes
Copy link
Contributor

@lipingxue lipingxue left a 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.

client_plugin/vmdk_plugin/sanity_test.go
@@ -16,6 +16,8 @@
// VMDK Docker driver sanity tests.
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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)".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

msterin
msterin approved these changes Jul 27, 2017
View reviewed changes
Copy link
Contributor

@msterin msterin left a 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)
Copy link
Contributor

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?"

Copy link
Contributor Author

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 \
Copy link
Contributor

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 ?

Copy link
Contributor Author

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!

@shuklanirdesh82 shuklanirdesh82 self-assigned this Jul 28, 2017
@pshahzeb
Copy link
Contributor

@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?

@shuklanirdesh82
Copy link
Contributor Author

#1633 (comment)

No @pshahzeb why do you think so?

@shuklanirdesh82 shuklanirdesh82 merged commit 39b4d08 into vmware-archive:master Jul 29, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the goTestCoverage.shuklanirdesh82 branch July 29, 2017 04:25
@pshahzeb
Copy link
Contributor

As per the offline discussion we had had over the issue of #1509, I was wondering if it would be possible to do so.

@shuklanirdesh82 shuklanirdesh82 mentioned this pull request Oct 2, 2017
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@msterin msterin msterin approved these changes

@lipingxue lipingxue lipingxue approved these changes

Assignees

@shuklanirdesh82 shuklanirdesh82

Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shuklanirdesh82 @pshahzeb @msterin @lipingxue @vmwclabot