Saggi Mizrahi has posted comments on this change. Change subject: tests: Add simple mocking library ......................................................................
Patch Set 1: > It does need testing, the file is a black box for us as you > said previously. We can change the implementation, but the > call to lvm with those parameters *is* our interface. If you declare an interface like that you're interface is wrong. It's like defining a function sum that says "adds 2 numbers using the operator +". You never do that you just say: "a function that returns the sum of 2 numbers" You don't check how it does that. You just check that the sum is correct. To reiterate. I don't say mocking is bad. I'm saying 2 things. 1. You can't use mocking for a wrapper method. If I have a method that wraps LVM calls I can't test it with mocked objects since what you want to make sure is that the wrapper actually works. If you want to test the parsing the simple way is to split the parsing to it's own method that is defined by how it interacts with text in the format that is returned by the tool. That way you don't break semantics and you don't need to use mock objects. If the output changes it is clear that we need a new function or that the actual semantic of the parsing function has to change. If we move to using something that doesn't require parsing the parsing method and it's tests can just be removed. This makes the whole process simple and straight forward. 2. mock objects in the vain of what you library offers are needlessly confusing. In python it's easy to make mock objects. instead of using a custom syntax for defining a method like: x = mock.callabale().expect(...).returns(...) You can do: def x(...): return... function definition contain all the bits that you mock objects give you just in a different, more standard, syntax. It's simpler, cleared and more concise to use functions and lambdas to achieve the same effect. --- Tests should never break if the function interface was not broken. ---- > Mock are programmable - the logic of which answer to > return is in the test. You don't need to write new > class for each test to simulate the specific flow. Functions and classes are programmable as well. In python, writing a class is not that bad. That goes back to my original argument about how a mock library is relevant for boilerplate heavy languages like Java\C#. -- To get back to your example. Lets take this test (http://gerrit.ovirt.org/#/c/20720/17/tests/toolLvmDeactivateLvsTests.py line:36) The only bit of actual code you test is: lvm_deactivate_lvs.lvm_deactivate_lvs() But you mock execCmd. WAAAAY down the stack. You should have mocked. _iter_vdsm_vgs() and _deactivate_lvs() Since the purpose of mocking is so that bugs in dependent code don't interfere with you testing. So what you could have done is _iter_vdsm_vgs = lambda : (vg for vg in ["a", "b", "c"] _deactivate_lvs = lambda a : pass Since you don't want errors\bugs\changes in dependent methods that don't effect the interface to break tests. again, the actual intention of mocking. As for _was_run() and _set_was_run(), this is a good example for trying to check for side effects. A correct mock object wouldn't have used a canned response. It would have used methods that actually depend on each other like the real _set_was_run() and _was_run() since what you are mocking is the "was_run" mechanism. I would have declared a bool in the method and made mock methods that change this bool instead of writing to a file. This way I actually mock the object. I can even declare the fake was_run method in the module itself and use them whenever needed. These would have made your test simpler to execute understand and would have used proper mocking. As the test stands it contains needless lvm arguments and output which is confusing and completely unrelated to what you are testing. -- To view, visit http://gerrit.ovirt.org/21155 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5d874f553b6a983652ed745d7d8554716e7a15e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
