Nir Soffer has posted comments on this change. Change subject: storage: support new dd output ......................................................................
Patch Set 1: (4 comments) https://gerrit.ovirt.org/#/c/56091/1/tests/Makefile.am File tests/Makefile.am: Line 138: storageMailboxTests.py \ Line 139: storageMonitorTests.py \ Line 140: storageServerTests.py \ Line 141: storagetestlibTests.py \ Line 142: storage_misc_test.py \ Did you look at miscTests.py? Line 143: storage_sdm_api_test.py \ Line 144: storage_sdm_create_volume_test.py \ Line 145: storage_volume_artifacts_test.py \ Line 146: tasksetTests.py \ https://gerrit.ovirt.org/#/c/56091/1/tests/storage_misc_test.py File tests/storage_misc_test.py: Line 23: Line 24: from monkeypatch import MonkeyPatchScope Line 25: Line 26: Line 27: class TestReadSpeed(VdsmTestCase): We already test this in miscTests.py:ReadSpeed.testReadSpeedRegExp. Can you integrated the tests there? Line 28: Line 29: def test_zero_size(self): Line 30: output = ( Line 31: "0+0 records in", Line 44: output = ( Line 45: "1+0 records in", Line 46: "1+0 records out", Line 47: "4096 bytes (4.1 kB, 4.0 KiB) copied, 0.00887814 s, 461 kB/s") Line 48: self.assertNotRaises(self._speed_from_output, output) assert not raises is not good enough, we want to get the time from that string (0.00887814). Line 49: Line 50: def test_read_30_bytes_human_readable(self): Line 51: output = ( Line 52: "0+1 records in", https://gerrit.ovirt.org/#/c/56091/1/vdsm/storage/misc.py File vdsm/storage/misc.py: Line 188: return str(ctime) Line 189: Line 190: _readspeed_regex = re.compile( Line 191: "(?P<bytes>\d+) bytes?" Line 192: "(|( \([\de\-.]+ [kMGT]*B(|, [\de\-.]+ [KMGT]*iB)\))) copied, " This probably works but too fragile. We don't care about the values which do not match now. I prefer to modify the old regex so we *ingore* the stuff we don't care about, and match only the stuff we care about. In the current code, we care about the number of bytes copied and number of seconds. Actually, we care only about the seconds value and we never use the bytes value. So the first change should be remove the unused bytes parsing. Then we can simplify the parsing by using string.split(). Line 193: "(?P<seconds>[\de\-.]+) s, " Line 194: "([\de\-.]+|Infinity) [kMGT]*B/s" Line 195: ) Line 196: -- To view, visit https://gerrit.ovirt.org/56091 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I632d99cdc2b41e96a75bdce12e86710241fa0939 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[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
