-
Notifications
You must be signed in to change notification settings - Fork 95
Capturing coverage of vmdkops service #1421
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.
LGTM.
vmdk_plugin/Makefile
Outdated
@@ -347,6 +347,26 @@ test-esx: | |||
testremote: test-esx test-vm | |||
test-all: test e2e-test testremote | |||
|
|||
# binary for python coverage | |||
COVERAGE := coverage | |||
#vmdkops service manager |
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.
Nit: add a space after #
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
vmdk_plugin/Makefile
Outdated
coverage: | ||
$(log_target) | ||
ifdef COV_EXIST | ||
@echo "coverage package exists." |
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.
Nit: coverage => python coverage
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
vmdk_plugin/Makefile
Outdated
$(SSH) root@$(ESX) '$(COVERAGE) report' | ||
$(SSH) root@$(ESX) 'rm .coverage*' | ||
else | ||
@echo "coverage package doesn't exist." |
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.
Same as above.
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.
Thanks for working on this quickly!
@@ -108,7 +108,7 @@ runtests) | |||
TARGET+=" e2e-test-runalways test-vm" | |||
else | |||
touch /tmp/$ESX | |||
TARGET+=" deploy-vm e2e-test-runalways e2e-test-runonce testasroot test-esx test-vm" | |||
TARGET+=" deploy-vm e2e-test-runalways e2e-test-runonce testasroot test-esx test-vm coverage" |
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.
we need to move this after L118
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.
added a switch case as per discussed offline
vmdk_plugin/Makefile
Outdated
@@ -347,6 +347,26 @@ test-esx: | |||
testremote: test-esx test-vm | |||
test-all: test e2e-test testremote | |||
|
|||
# binary for python coverage |
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.
let's not pollute this file .. this target should move to esx_service Makefile.
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
@pshahzeb Please trigger the CI run for this PR.
Good to have some information from local run target. A quick question: what happens incase of error while capturing the coverage data (fail the PR test run or ignores silently)? |
Where are changes documented? What additional configuration is required if I need to run this on my local ESX.
Please add details like target compiled, output of coverage etc. |
2885ded
to
0ee9a45
Compare
If the tests passed, coverage capture should not fail. If ever it does, that means |
Updating contributing.md
0ee9a45
to
23ca315
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.
LGTM! You could cut/paste coverage info from CI run in the PR description or paste the CI run URL along with local run for future reference.
Supplying minor nits to address.
vmdk_plugin/Makefile
Outdated
@@ -347,6 +347,11 @@ test-esx: | |||
testremote: test-esx test-vm | |||
test-all: test e2e-test testremote | |||
|
|||
# capture coverage of ESX Python code. |
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.
minor: capture coverage of ESX Python code
=> capture coverage of vDVS driver code
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
CONTRIBUTING.md
Outdated
User can configure the setup and use this target to see the coverage. | ||
### Setup ESX box to install python coverage package | ||
* Install https://coverage.readthedocs.io/en/coverage-4.4.1/ on your machine using pip | ||
- scp the coverage package dir installed on ur machine i.e. <Python Location>/site-packages/coverage to /lib64/python3.5/site-packages/ on ESX box. |
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.
would suggest to add the sample command instead of text .. sample command would be handy. Command reference would be easy. :)
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.
added exact commands
CONTRIBUTING.md
Outdated
- scp the coverage package dir installed on ur machine i.e. <Python Location>/site-packages/coverage to /lib64/python3.5/site-packages/ on ESX box. | ||
- scp the coverage binary i.e. /usr/local/bin/coverage to /bin/ on ESX box | ||
* create sitecustomize.py inside /lib64/python3.5/ | ||
The content of sitecustomize.py is |
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.
nit: need one more line (\n
) ... it is appending with L389
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
Relaxing args check for CI deploy and test script
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.
Some minor doc comments.
CONTRIBUTING.md
Outdated
@@ -378,3 +378,31 @@ Test SSH keys, login form the drone node should not require typing in a password | |||
cd $GOPATH/src/github.com/vmware/docker-volume-vsphere/ | |||
drone exec --trusted --yaml .drone.dev.yml -i ~/.ssh/id_rsa -e VM1=<ip VM1> -e VM2=<ip VM2> -e ESX=<ip ESX> | |||
``` | |||
|
|||
## Capturing coverage |
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.
ESX Code Coverage
CONTRIBUTING.md
Outdated
Coverage is captured using make coverage target (On CI it is called using drone script). | ||
User can configure the setup and use this target to see the coverage. | ||
### Setup ESX box to install python coverage package | ||
* Install https://coverage.readthedocs.io/en/coverage-4.4.1/ on your machine using pip |
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.
Add exact command and prefix commands to run on ESX with ESX$
or laptop with Desktop$
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
CONTRIBUTING.md
Outdated
### Setup ESX box to install python coverage package | ||
* Install https://coverage.readthedocs.io/en/coverage-4.4.1/ on your machine using pip | ||
- scp the coverage package dir installed on ur machine i.e. <Python Location>/site-packages/coverage to /lib64/python3.5/site-packages/ on ESX box. <br /> | ||
eg:- scp -r /Library/Python/3.5/site-packages/coverage root@$ESX:/lib64/python3.5/site-packages/ |
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.
Include commands in command-in-single-quote
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! good to go after addressing open comments.
From CI https://ci.vmware.run/vmware/docker-volume-vsphere/565.
|
https://ci.vmware.run/vmware/docker-volume-vsphere/582 pass with coverage indicated.. Merging this. |
This reverts commit d735de5.
CI ESX box is patched to have python coverage package.
Testing: Done locally.
Following is the end of:
make build-all deploy-all test-all coverage
Fixes #404