Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 8: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2782/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1970/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2856/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 8: Fails built rpms installed: yum install vdsm-4.11.0-30.giteb92231.el6.x86_64.rpm vdsm-cli-4.11.0-30.giteb92231.el6.noarch.rpm vdsm-xmlrpc-4.11.0-30.giteb92231.el6.noarch.rpm vdsm-python-4.11.0-30.giteb92231.el6.x86_64.rpm vdsm-coverage-plugin-4.11.0-30.giteb92231.el6.noarch.rpm vdsm-python-cpopen-4.11.0-30.giteb92231.el6.x86_64.rpm cat /etc/vdsm/coveragerc [run] branch = True data_file = /var/lib/vdsm/coverageresults disabled: codeCoveragePlugin::43::code_coverage::(instrument) Code coverage is disabled. enabled: codeCoveragePlugin::53::code_coverage::(instrument) Starting vdsm code coverage with /etc/vdsm/coveragerc but the results file wasn't generated, because if we use coverage.start(), then we need to also call coverage.stop() in order to generate it. coverage.process_startup() do both automatically. I can go back to coverage.process_startup() function, or create singleton class and call coverage.save() in __del__ . or I would be glad for other your thoughts. -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 7: Looks good to me, approved -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 7: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2780/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1968/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2854/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change.
Change subject: Added code coverage plugin
..
Patch Set 6: I would prefer that you didn't submit this
(2 inline comments)
File lib/vdsm/config.py.in
Line 269:
Line 270: # Section: [code_coverage]
Line 271: ('code_coverage', [
Line 272:
Line 273: ('enabled', 'false', None),
please spend a few seconds to document the new elements. None -> "set to True
to enable code coverage measurements"
Line 274:
Line 275: ('config_file', '', None),
Line 276:
Line 277: ]),
Line 271: ('code_coverage', [
Line 272:
Line 273: ('enabled', 'false', None),
Line 274:
Line 275: ('config_file', '', None),
Is the empty string a reasonable value? I think that something like
/etc/vdsm/coveragerc
makes more sense.
Line 276:
Line 277: ]),
Line 278: ]
Line 279:
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Lukas Bednar
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 6: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2779/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1967/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2853/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Yaniv Bronhaim has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 5: Dan, don't you prefer to have default entries for coverage in config.py.in? -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 3: (2 inline comments) A race condition made me not post these comments but they still apply. File vdsm/codeCoveragePlugin.py Line 55: return Line 56: Line 57: config_file = None Line 58: if config.has_option(SECTION_NAME, CONFIG_FILE): Line 59: config_file = config.get(SECTION_NAME, CONFIG_FILE) instead of all these checks, I suggest that you do what we anywhere else in vdsm: write a default in config.py. Thus, you only need config.getboolean(SECTION, ENABLED) Line 60: Line 61: start_msg = "Starting vdsm code coverage" Line 62: if config_file and os.path.exists(config_file): Line 63: os.environ['COVERAGE_PROCESS_START'] = config_file Line 59: config_file = config.get(SECTION_NAME, CONFIG_FILE) Line 60: Line 61: start_msg = "Starting vdsm code coverage" Line 62: if config_file and os.path.exists(config_file): Line 63: os.environ['COVERAGE_PROCESS_START'] = config_file In my opinion, it is much nicer to have (untested) from coverage.control import coverage coverage(config_file=config_file, auto_data=True).start() as using env variable to communicate with one process is usually not recommended. Line 64: start_msg = "%s with %s" % (start_msg, config_file) Line 65: Line 66: from coverage.control import process_startup Line 67: log.warn(start_msg) -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 5: I would prefer that you didn't submit this Please see my comments to ps3 -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 5: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2758/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1946/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2832/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 4: I would prefer that you didn't submit this -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 4: Fails; I would prefer that you didn't submit this Build Failed http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2757/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1945/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2831/ : FAILURE -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 3: (2 inline comments) File vdsm/codeCoveragePlugin.py Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: """ Line 20: This module is meant to be imported by vdsm in order to enable code coverage. Line 21: You can configure it in the configuration file: I was considering adding defaults there, but I found that as unnecessary since this module should be on production machines. Line 22: Line 23: [code_coverage] Line 24: enabled = true Line 25: config_file = /path/to/config/file/for/python-coverage/module Line 31: Line 32: Line 33: ENABLED = 'enabled' Line 34: CONFIG_FILE = 'config_file' Line 35: SECTION_NAME = 'code_coverage' I don't know, I am just used to declare module related constants on module level. same as class related consts as class members. Line 36: Line 37: Line 38: def instrument(): Line 39: """ -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Yaniv Bronhaim has posted comments on this change.
Change subject: Added code coverage plugin
..
Patch Set 3: I would prefer that you didn't submit this
(4 inline comments)
File vdsm/codeCoveragePlugin.py
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: """
Line 20: This module is meant to be imported by vdsm in order to enable code
coverage.
Line 21: You can configure it in the configuration file:
"To enable code coverage add to vdsm.conf the following:"
and you should also add defaults to lib/vdsm/config.py.in
Line 22:
Line 23: [code_coverage]
Line 24: enabled = true
Line 25: config_file = /path/to/config/file/for/python-coverage/module
Line 31:
Line 32:
Line 33: ENABLED = 'enabled'
Line 34: CONFIG_FILE = 'config_file'
Line 35: SECTION_NAME = 'code_coverage'
why globals? do you use it anywhere else except from here? please put those
inside instrument()
Line 36:
Line 37:
Line 38: def instrument():
Line 39: """
Line 42:
Line 43: log = logging.getLogger("code_coverage")
Line 44:
Line 45: if not config.has_section(SECTION_NAME):
Line 46: log.warn("Code coverage is not configured.")
could be:
def instrument():
section_name = 'code_coverage'
config_file = 'config_file'
enabled = 'enabled'
if not config.has_section(section_name) or not config.has_option(section_name,
enabled):
log.info("Code coverage is not configured in vdsm.conf")
if not tobool(config.get(SECTION_NAME, ENABLED)):
log.info("Code coverage is disabled")
...read and add config_file to environ dict...
log.info more fits here..
Line 47: return
Line 48:
Line 49: enabled = False
Line 50: if config.has_option(SECTION_NAME, ENABLED):
File vdsm/vdsm
Line 75: # Any other error is an error in the debug
Line 76: # plugin and we would like to print that out.
Line 77: pass
Line 78:
Line 79: # Used to enable code coverage. on production machines
sorry, i don't like the above message either.. but this is new code and the py
file exists only if you install coverage-plugin rpm. so to avoid digging and
finding where this file comes from just write:
"Used to enable code coverage if coverage-plugin.rpm is installed"
Line 80: # codeCoveragePlugin.py should not exists
Line 81: try:
Line 82: from codeCoveragePlugin import instrument
Line 83: instrument()
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Lukas Bednar
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 3: Verified built rpms, installed, checked whether coverage was generated -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 3: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2753/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1941/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2827/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has posted comments on this change.
Change subject: Added code coverage plugin
..
Patch Set 2: (1 inline comment)
File vdsm/codeCoveragePlugin.py
Line 37: "%s variable to enable it", OPTION_NAME)
Line 38: return
Line 39:
Line 40: config = None
Line 41: enabled = os.environ[OPTION_NAME].strip()
see several lines bellow.
Line 42:
Line 43: if enabled.lower() not in POSITIVE_ANSWERS:
Line 44: if not os.path.exists(enabled):
Line 45: log.warn("Found %s=%s; but expected is one from %s options
"
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Lukas Bednar
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Yaniv Bronhaim has posted comments on this change.
Change subject: Added code coverage plugin
..
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
File vdsm/codeCoveragePlugin.py
Line 22: import os
Line 23: import logging
Line 24:
Line 25:
Line 26: log = logging.getLogger("code_coverage")
you can move the log part inside instrument function. and both OPTION_NAME and
POSITIVE_ANSWER can be omitted.
I don't see reason for globals here..
Line 27: OPTION_NAME = 'VDSM_CODE_COVERAGE'
Line 28: POSITIVE_ANSWERS = ('yes', 'true', 'on', '1')
Line 29:
Line 30:
Line 37: "%s variable to enable it", OPTION_NAME)
Line 38: return
Line 39:
Line 40: config = None
Line 41: enabled = os.environ[OPTION_NAME].strip()
enabled is /path/to/config/file/for/python-coverage/module as you mentioned in
your comment message. how is it in POSITIVE_ANSWERS? maybe i missed something..
Line 42:
Line 43: if enabled.lower() not in POSITIVE_ANSWERS:
Line 44: if not os.path.exists(enabled):
Line 45: log.warn("Found %s=%s; but expected is one from %s options
"
File vdsm/vdsm
Line 81: try:
Line 82: import codeCoveragePlugin
Line 83: except ImportError:
Line 84: # This is OK, it just means the file isn't
Line 85: # there and we are not running in coverage mode.
IMHO, this comment is redundant.. its quite obvious what this code does and you
don't need to repeat the comment above.
you can add above the try:
# Importing coverage package if coverage-plugin is installed. otherwise,
continue regularly .
But I don't think that a comment is required here at all.
Line 86: pass
Line 87:
Line 88: log = logging.getLogger('vds')
Line 89: try:
File vdsm/vdsmd.init.in
Line 22: ### END INIT INFO
Line 23:
Line 24: . @LIBEXECDIR@/ovirt_functions.sh
Line 25:
Line 26: [ -f /etc/default/vdsmd ] && . /etc/default/vdsmd
could be:
coverage=`$GETCONFITEM $CONF_FILE vars coverage disabled | tr A-Z a-z`
if [ "$coverage" = "enabled" ]; then
VDSM_CODE_COVERAGE=/path/to/config/file/for/python-coverage/module
fi
First, I'm not sure that the env variable couldn't be declared as part of
python-coverage rpm installation
(http://unix.stackexchange.com/questions/45540/how-to-automatically-add-new-path-to-path-variable-after-rpm-packet-installatio).
you can add %post coverage-plugin section in vdsm.spec
Second, if you declare it here, I'm not sure if those env variables are passed
to vdsm user.
Third, you can use dan's suggestion and move it to the module itself.
Line 27:
Line 28: VDSM_BIN=@VDSMDIR@/vdsm
Line 29: CONF_FILE=@CONFDIR@/vdsm.conf
Line 30: GETCONFITEM=@VDSMDIR@/get-conf-item
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Lukas Bednar
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Dan Kenigsberg has posted comments on this change.
Change subject: Added code coverage plugin
..
Patch Set 2: I would prefer that you didn't submit this
(3 inline comments)
Thanks, but I think that the code can become much simpler if you do
from config import config
if config.getbool('coverage'):
import coverage.
process_start()
File vdsm/codeCoveragePlugin.py
Line 39:
Line 40: config = None
Line 41: enabled = os.environ[OPTION_NAME].strip()
Line 42:
Line 43: if enabled.lower() not in POSITIVE_ANSWERS:
We have until.tobool for that.
Line 44: if not os.path.exists(enabled):
Line 45: log.warn("Found %s=%s; but expected is one from %s options
"
Line 46: "or path to existing config file.", OPTION_NAME,
enabled,
Line 47: POSITIVE_ANSWERS)
Line 59: from coverage.control import process_startup
Line 60: process_startup()
Line 61:
Line 62:
Line 63: instrument()
Running a function on import is a bad practice. For example, it makes unit
tests harder. Let the importer explicitly call the function.
File vdsm/vdsmd.init.in
Line 22: ### END INIT INFO
Line 23:
Line 24: . @LIBEXECDIR@/ovirt_functions.sh
Line 25:
Line 26: [ -f /etc/default/vdsmd ] && . /etc/default/vdsmd
Coverage is not specific to sysv service. Please read the variable from
/etc/vdsm/vdsm.conf like any other configurable. It would simplify your code,
too.
Line 27:
Line 28: VDSM_BIN=@VDSMDIR@/vdsm
Line 29: CONF_FILE=@CONFDIR@/vdsm.conf
Line 30: GETCONFITEM=@VDSMDIR@/get-conf-item
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Lukas Bednar
Gerrit-Reviewer: Yaniv Bronhaim
Gerrit-Reviewer: oVirt Jenkins CI Server
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 2: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2746/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1934/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2820/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 2: Verified built rpms, installed, and checked whether coverage_report was generated. -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Lukas Bednar Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
oVirt Jenkins CI Server has posted comments on this change. Change subject: Added code coverage plugin .. Patch Set 1: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/2744/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/1932/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/2818/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/15545 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Lukas Bednar Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Yaniv Bronhaim Gerrit-Reviewer: oVirt Jenkins CI Server ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Added code coverage plugin
Lukas Bednar has uploaded a new change for review.
Change subject: Added code coverage plugin
..
Added code coverage plugin
This commit adds option to enable vdsm code coverage.
If you want to enable it you need to create file /etc/default/vdsmd with
following content and restart vdsm.
VDSM_CODE_COVERAGE = /path/to/config/file/for/python-coverage/module
Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Signed-off-by: Lukas Bednar
---
M vdsm.spec.in
A vdsm/codeCoveragePlugin.py
M vdsm/vdsm
M vdsm/vdsmd.init.in
4 files changed, 86 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/45/15545/1
diff --git a/vdsm.spec.in b/vdsm.spec.in
index b0e3b87..e945e3d 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -293,6 +293,14 @@
%description debug-plugin
Used by the trained monkeys at Red Hat to insert chaos and mayhem in to VDSM.
+%package coverage-plugin
+Summary:VDSM Code Coverage Plugin
+Requires: python-coverage >= 3.5.3
+BuildArch: noarch
+
+%description coverage-plugin
+Used by automation tests to measure code coverage.
+
%package tests
Summary:VDSM Test Suite
Requires: vdsm = %{version}-%{release}
@@ -1123,6 +1131,10 @@
%{_datadir}/%{vdsm_name}/vdsmDebugPlugin.py*
%{_datadir}/%{vdsm_name}/debugPluginClient.py*
+%files coverage-plugin
+%defattr(-, root, root, -)
+%{_datadir}/%{vdsm_name}/codeCoveragePlugin.py*
+
%files cli
%defattr(-, root, root, -)
%doc COPYING
diff --git a/vdsm/codeCoveragePlugin.py b/vdsm/codeCoveragePlugin.py
new file mode 100644
index 000..b12306d
--- /dev/null
+++ b/vdsm/codeCoveragePlugin.py
@@ -0,0 +1,63 @@
+#
+# Copyright 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+#
+# Refer to the README and COPYING files for full details of the license
+"""
+This module is meant to be imported by vdsm in order to enable code coverage
+"""
+import os
+import logging
+
+
+log = logging.getLogger("code_coverage")
+OPTION_NAME = 'VDSM_CODE_COVERAGE'
+POSITIVE_ANSWERS = ('yes', 'true', 'on', '1')
+
+
+def instrument():
+"""
+enables code coverage, you can specify config file for coverage module
+"""
+if OPTION_NAME not in os.environ:
+log.warn("Code coverage is dissabled. You need to export "
+ "%s variable to enable it", OPTION_NAME)
+return
+
+config = None
+enabled = os.environ[OPTION_NAME].strip()
+
+if enabled.lower() not in POSITIVE_ANSWERS:
+if not os.path.exists(enabled):
+log.warn("Found %s=%s; but expected is one from %s options "
+ "or path to existing config file.", OPTION_NAME, enabled,
+ POSITIVE_ANSWERS)
+log.warn("Code coverage is going to be skipped")
+return
+config = enabled
+
+start_msg = "Starting vdsm code coverage"
+if config:
+log.warn("%s with %s", start_msg, config)
+os.environ['COVERAGE_PROCESS_START'] = config
+else:
+log.warn(start_msg)
+
+from coverage.control import process_startup
+process_startup()
+
+
+instrument()
diff --git a/vdsm/vdsm b/vdsm/vdsm
index fef86db..0f945a0 100755
--- a/vdsm/vdsm
+++ b/vdsm/vdsm
@@ -76,6 +76,15 @@
# plugin and we would like to print that out.
pass
+# Used to enable code coverage. on production machines
+# codeCoveragePlugin.py should not exists
+try:
+import codeCoveragePlugin
+except ImportError:
+# This is OK, it just means the file isn't
+# there and we are not running in coverage mode.
+pass
+
log = logging.getLogger('vds')
try:
logging.root.handlers.append(logging.StreamHandler())
diff --git a/vdsm/vdsmd.init.in b/vdsm/vdsmd.init.in
index 7a6d2db..2a9a397 100755
--- a/vdsm/vdsmd.init.in
+++ b/vdsm/vdsmd.init.in
@@ -23,6 +23,8 @@
. @LIBEXECDIR@/ovirt_functions.sh
+[ -f /etc/default/vdsmd ] && . /etc/default/vdsmd
+
VDSM_BIN=@VDSMDIR@/vdsm
CONF_FILE=@CONFDIR@/vdsm.conf
GETCONFITEM=@VDSMDIR@/get-conf-item
--
To view, visit http://gerrit.ovirt.org/15545
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6cb465f7723abad9698aa86bedf2ffc25df55713
Gerri
