[Pharo-dev] Use #shouldnt:raise: only with specific errors, or evaluate the given expression directly

Igor Stasenko siguctua at gmail.com
Tue Oct 22 06:39:11 EDT 2013


On 22 October 2013 10:02, Camillo Bruni <camillobruni at gmail.com> wrote:

> 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.


+1
'shouldn't raise' is bogus and as you said, it is default expectation for
any code
you run that it doesn't throws any errors.
(that's why exceptions called so and that's why error is exception).

i can understand why i would want to use 'should raise' to check that my
code
throws appropriate error when i using wrong data etc.. but not this one.


> 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
> for those.
>
> 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
>
> vs.
>         0/0.
>
>
> Of course we can change it back at some point, but I haven't heard a solid
> reason in favor of shouldnt:raise:Error.
>
> > Johan
> >
> > 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:
> >>>
> >>> -=-=-=-=-=-=-=-=-=
> >>> testNoErrorWhenDrawing
> >>>     self shouldnt: [ view raw drawOn: tracingCanvas ] raise: Error
> >>> -=-=-=-=-=-=-=-=-=
> >>>
> >>> ||
> >>> V
> >>>
> >>> -=-=-=-=-=-=-=-=-=
> >>> testNoErrorWhenDrawing
> >>>     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 :)
> >
> >
>
>


-- 
Best regards,
Igor Stasenko.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.pharo.org/pipermail/pharo-dev_lists.pharo.org/attachments/20131022/05d63470/attachment-0002.html>


More information about the Pharo-dev mailing list