Milan Zamazal has posted comments on this change. Change subject: tests: improve vmstats.disks coverage ......................................................................
Patch Set 4: (1 comment) I think the tests checking for missing stats are intended to report that corresponding KeyError's are raised instead of being handled (as they are in your related stats robustness patches). They don't report that although the KeyError's are actually raised. The problem happens in vmstats._skip_if_missing_stats: It receives the exception and calls vm_obj.incomingMigrationPending(). However fake VM doesn't contain incomingMigrationPending() so AttributeError is raised and probably caught elsewhere. Thus KeyError is discarded and the test passes although it shouldn't. This is not exactly problem of this change (if incomingMigrationPending() was present and working the missing stats test would fail), but should be fixed before we claim we code is well covered with the tests. https://gerrit.ovirt.org/#/c/50598/4/tests/vmStatsTests.py File tests/vmStatsTests.py: Line 462: # helpers Line 463: Line 464: def _ensure_delta(stats_before, stats_after, key, delta): Line 465: """ Line 466: make sure that (after[key] - before[key]) == delta. No, what this actually does is: Set stats_before[key] and stats_after[key] so that stats_after[key] - stats_before[key] == abs(delta). Line 467: """ Line 468: stats_before[key] = 0 Line 469: stats_after[key] = abs(delta) Line 470: -- To view, visit https://gerrit.ovirt.org/50598 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I931d080d66b8a6087b05f01fbdce8fee0cd99039 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Milan Zamazal <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
