Saggi Mizrahi has posted comments on this change.
Change subject: tests: Add simple mocking library
......................................................................
Patch Set 1: Code-Review-1
Sorry to be the one to rain on your parade but I don't really understand what
this is for. I'll repeat what I said in you previous iteration of this module.
I don't understand why one would need to check if things are called with
certain paramters. This makes the test assume things about the method being
called that are not part of it's declared interface.
You should never check if something calls something else or if it uses certain
parameters. The algorithm of the method is malleable and tests should not fail
if you change it. Only logic should be tested and verified.
When you test things like that you are actually just making it so that the
person changing the algorithm needs to change the test as well even when they
fix things.
Lets say we need to change a certain parameter to a called function so that the
actual interface is fixed. In that case, even though we fix the method the test
would fail.
Functions are black boxes as far as unit tests are concerned. By ignoring that
you make the developer confused whether they actually broke something or if the
test is just there to give him extra work.
For the use case of making stubs easier.
Stubs usually need to have logic and state.
So you'll probably implement you own classes.
Further more, making a function that makes for functions is needlessly
convoluted.
for instance to make a function that returns a value you can do
def foo():
return "abc"
This syntax though a bit more verbose is clear and well understood by all
programmers. It also uses python which means you can add extra logic as needed.
So instead of c = mock.Callable().expect("foo").result("bar")
you can de
def c(s): self.assertEquals("foo", s); return "bar"
which is only 3 chars longer (If I remove the optional space chars it is
actually shorter) but doesn't require someone to learn your DSL and is
endlessly extensible.
I would have given a -2 if there weren't so many +1s already.
--
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
