Change in vdsm[master]: tests: Add simple mocking library

smizrahi at redhat.com smizrahi at redhat.com
Sun Dec 15 13:19:23 UTC 2013


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 <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No


More information about the vdsm-patches mailing list