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. 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. Johan On 22 Oct 2013, at 00:26, Camillo Bruni <[email protected]> wrote: > > On 2013-10-22, at 00:20, Alexandre Bergel <[email protected]> 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 :)
