Tests don't test what you think they're testing.

[Robert Bruce Park]

Ok, so I'm just analyzing the compiler warnings and trying to clean stuff up, when I was confronted with this little curio:

test-service.vala:73.19-76.13: warning: unreachable catch clause detected
test-service.vala:90.19-93.13: warning: unreachable catch clause detected
test-service.vala:106.19-109.13: warning: unreachable catch clause detected
test-service.vala:141.19-144.13: warning: unreachable catch clause detected
test-service.vala:159.19-162.13: warning: unreachable catch clause detected
test-service.vala:178.19-181.13: warning: unreachable catch clause detected
test-service.vala:194.19-197.13: warning: unreachable catch clause detected
test-service.vala:209.19-212.13: warning: unreachable catch clause detected

Upon further investigation, it makes sense: The methods being tested catch those exceptions themselves, and the compiler is correctly identifying that these catch blocks can literally never be triggered under any circumstances. This makes the testsuite effectively *tautological*: We run a method that can't fail, then we set a variable to true, then we assert that the true value is true. There is simply no possible condition under which this test can fail, even if the libfriends code that it purportedly tests is utterly broken, the test will still pass.

What we *really* need to be doing in this testsuite is as simple as confirming that the vala methods are invoking the dbus methods. That's literally all that needs to get done (because all the *real* functionality is already implemented and tested on the python side). I scoured through the dbusmock documentation, and it seems that dbusmock unfortunately does not provide any kind of dbus method that could be considered comparable to python's mock.assert_called_once_with() method, it only provides mock-creation methods.

So how do we go about confirming that methods have been called? Well, in pitti's official documentation, he says that all mocked dbus methods spew out their call signature on STDOUT, and indeed all the official examples demonstrate diverting STDOUT into a special log, that is then searched for the expected textual output.

Unfortunately I don't know enough about vala to do this STDOUT redirection myself (all of the examples given were pure python only), but sure enough in the 'make check' output you can clearly *see* this logging output:

DBus daemon: unix:abstract=/tmp/dbus-DvfzGH4nLA,guid=433c20c97b4eb2910756f1f550f0bc98
DBusService: Started with PID: 31108
Service : Started with PID: 31112
Activating service name='org.a11y.Bus'
Successfully activated service 'org.a11y.Bus'
Activating service name='org.a11y.atspi.Registry'
Service : /Unit/Friends/Service/Refresh: OK

DBusService: 1357954200.392 Refresh

Service : /Unit/Friends/Service/Quit: OK

DBusService: 1357954200.398 Quit

Service : /Unit/Friends/Service/Features: OK

Service : /Unit/Friends/Service/SendMessage: OK

DBusService: 1357954200.404 GetFeatures "twitter"

DBusService: 1357954200.417 SendMessage "A message"

Service : /Unit/Friends/Service/SendMessageAccout: OK

DBusService: 1357954200.423 Do "send" "1" "A message"

Service : /Unit/Friends/Service/SendReply: OK

DBusService: 1357954200.428 SendReply "A message" "1" "100"

Service : /Unit/Friends/Service/ClearIndicators: OK

Service : /Unit/Friends/Service/URLShorten: OK

DBusService: 1357954200.433 ClearIndicators

DBusService: 1357954200.439 URLShorten "http://example.com/really/really/long"

Service : /Unit/Friends/Utils/time_string_seconds: OK

Service : /Unit/Friends/Utils/time_string_minute: OK

Service : /Unit/Friends/Utils/time_string_minutes: OK

Service : /Unit/Friends/Utils/time_string_hour: OK

DBusService: Shutting down
Service: Shutting down
DBus daemon: Shutdown
PASS: test-runner

So Ken, I'm gonna need you to modify the testrunner to divert STDOUT into some kind of buffer that is accessible to the testsuite, and then all the calls to `assert(success == true)` are gonna need to be changed to something more along the lines of asserting that the above log lines appear in the diverted log output.

Hope this makes sense! Let me know if you need to know anything else to fix this.

[Ken VanDine (ken-vandine)]

Actually I think the problem is the functions being tested need to not only catch the error but also throw an error for the tests or clients to catch.

[Robert Bruce Park (robru)]

Hmmmm. Well if we are going to throw an error, why bother catching & rethrowing at all? Why not just let the existing exceptions bubble up to the test/client?

But, we still need to capture dbusmock's STDOUT and confirm the presents of the method call signatures there. That is the only way to confirm that it is actually calling the correct dbus method. A testsuite that simply says "ok, it didn't throw any errors, it must be working properly then" is not good enough. What if the method is empty, doesn't raise any exceptions, but also doesn't call the correct dbus method? We definitely need to confirm that the dbus methods are being invoked with the correct signature and the only way to do that is by capturing STDOUT from dbusmock and examining it's contents.

[Ken VanDine (ken-vandine)]

The dbus functions from friends-service will throw IOError if there is a failure, which contains the error message from the service. There is no output, even on stdout for success. I think catching the error is enough. For the test suite, I agree there is no value in that since we are mocking it anyway, so it will always succeed. However, what we do gain from that is testing for API changes. Which is why ideally I would want the dbusmock to be provided by friends-service instead of contained in libfriends. When python-dbusmock supports loading templates from other packages, we should create a dbusmock template that comes from friends and load that for the interfaces in the libfriends test suite.

[Robert Bruce Park (robru)]

Not sure I follow you here.

libfriends is a *very* light wrapper around the existing dbus API, and as such when we mock out dbus with dbusmock, the only thing that exists to test is whether or not we are calling the dbus API correctly. So if we don't capture the STDOUT produced by dbusmock and verify its correctness, then the testsuite is doing nothing. It's tautological. "We define this to be true. Is it true? Ok, it's true: success!"

I agree with you that it would be best to package the mock API with friends instead of libfriends, but I'm not really sure how to do this. I talked with pitti about it, and he doesn't plan to add any support for loading 3rd party templates. He says templates are specifically for mocking out common system things, so you can mock out eg UPower and test an app that toys with power settings, without actually causing your laptop to sleep while running the testsuite.

So, seeing as it's unlikely that dbusmock is going to allow 3rd party templates, I really don't see any options here. If we don't catch STDOUT, then we simply aren't using dbusmock correctly, and the tests are not testing anything at all.

[Robert Bruce Park (robru)]

Ok, so trying to pipe dbusmock STDOUT into vala test was kind of a bust due to dbus-test-runner wrapping everything and not really supporting that feature. So instead, I submitted a patch to dbusmock that gives it two new dbus methods: one for clearing the call log, and one for querying the call log. Once pitti merges that we'll be able to move forward with improving the libfriends test suite.