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 <lbed...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Lukas Bednar <lbed...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
