[Pharo-dev] Use #shouldnt:raise: only with specific errors, or evaluate the given expression directly
camillobruni at gmail.com
Tue Oct 22 04:02:52 EDT 2013
On 2013-10-22, at 09:46, Johan Brichau <johan at inceptive.be> wrote:
> In Seaside, there are a couple of tests that now fail because of this change.
> Although I agree it's good practice to use specific error classes in such assertions, the tests that are failing in Seaside use the Error class because they are written to be cross-platform. Different subclasses of Error will be thrown on different platforms. Moreover, the use of the assertion specifically documents that this test is testing for any errors being thrown. Removing the assertion means we need to write it down in comments, which is less explicit imho.
That's not true, that is the default behavior that you test for not Errors.
I mean every code that is run must work and testing that it does not raise
and Error is duplicated efforts. As I said, checking for any other error makes
somehow sense, but not for Error itself. Because that basically means that you
ignore what the test does since you are not specific at all. The test framework
takes already care of that part, if you have an error the test fails. I think
that is the goal. I do not understand why you would have to explain this to the
user of the tests, since for him the result does not change. Yes it is red
instead of yellow, but does that matter? I mean you just transformed at will an
error into an assertion failure, to me this sounds very bogus. Just because you
transform the error to an assertion, you did not anticipated anything, since you
cannot be sure at all where the error happened, since you do not test for an
explicit error. On the other hand if you call a method that is very small to check
if it really works, you know which errors are signaled and you test explicitly
I think I can come up with a tree transformation rule to go from #shouldnt:raise:Error
to a refactored version that even includes a comment.
> I'm not in favour of enforcing best practices this way. It just yields more workarounds in the end.
> I also do not understand the problem with debugging the test. If the test fails, you step through it and will discover the cause.
But if you just use the expression directly, the debugger pops up in the place where
the error was signaled. Not somewhere in deep in the assertion framework. Furthermore
if you run the test under jenkins you will get no reasonable feedback (yes this should
be changed as well). Just try to debug the following statement in a test:
self shouldnt: [ 0/0 ] raise: Error
Of course we can change it back at some point, but I haven't heard a solid reason in favor of shouldnt:raise:Error.
> On 22 Oct 2013, at 00:26, Camillo Bruni <camillobruni at gmail.com> wrote:
>> On 2013-10-22, at 00:20, Alexandre Bergel <alexandre.bergel at me.com> wrote:
>>> Instead of using shouldnt:raise:, you can simply remove the assertion, as in:
>>> self shouldnt: [ view raw drawOn: tracingCanvas ] raise: Error
>>> view raw drawOn: tracingCanvas
>>> With the second version of the test, the test may be listed as an error in case of an exception, whereas the first version it can only be listed as a failure.
>> Right, but that doesn't make any difference IMO, using `shouldnt: [...] raise: Error` to not have errors
>> does not make much sense. It makes a little bit more sense when using a specific Error, and it
>> makes sense when testing for Notifications, which would go otherwise unnoticed.
>> Which is the reason that `#shouldnt:raise:` no longer accepts `Error` as an argument, but still lets
>> you use anything else.
>>> I read your post and I kind of agree.
>>> I will fix my tests then.
>> nice :)
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 447 bytes
Desc: Message signed with OpenPGP using GPGMail
More information about the Pharo-dev