BlockContextTest >> #testNew

testNew
    self should: [ContextPart new: 5] raise: Error.
    [ContextPart new: 5]
        ifError: [:error :receiver | error = 'Error: Contexts must
only be created with newForMethod:'].
    [ContextPart new]
        ifError: [:error :receiver | error = 'Error: Contexts must
only be created with newForMethod:'].
    [ContextPart basicNew]
        ifError: [:error :receiver | error = 'Error: Contexts must
only be created with newForMethod:'].

Surely those "error = " lines need asserts?

frank

Yea, the code definitely appears wrong for testing, but is it really
useful to assert the *exact wording* of an error message, and
duplicate it 4 times?

I regard the messageText of Exceptions to be "for human consumption
only".  Any time I see code that thinks its necessary to check the
wording of an exception makes me want to say, "If you think you need
to do that then you your own Exception subclass."

So, of the 4 tests in this method, I think the second one should be
removed, and the last two should be made like the first one (e.g.,
without the string literals).

On Thu, Jan 9, 2014 at 7:30 AM, Frank Shearar <[hidden email]> wrote:

On 9 January 2014 16:25, Chris Muller <[hidden email]> wrote:
> Yea, the code definitely appears wrong for testing, but is it really
> useful to assert the *exact wording* of an error message, and
> duplicate it 4 times?
>
> I regard the messageText of Exceptions to be "for human consumption
> only".  Any time I see code that thinks its necessary to check the
> wording of an exception makes me want to say, "If you think you need
> to do that then you your own Exception subclass."

I do check error messages, but I only check for the presence of
particular things. Like the Inbox submission for SUnit I just posted,
I want to know that the messageText says "not identical". In other
words, I like to test that an error message is _useful_.

> So, of the 4 tests in this method, I think the second one should be
> removed, and the last two should be made like the first one (e.g.,
> without the string literals).

I'm with you on the removal of duplication. Personal preference would
be to say something like self assert: (e messageText
includesSubString: '#newForMethod').

I couldn't get #ifError: to work properly in a test, so for my
#assert:identical: tests I had a #should:raise: and then an on:do:
catching the same type of exception, and verifying the usefulness of
the error message.

frank

On Thu, Jan 9, 2014 at 10:34 AM, Frank Shearar <[hidden email]> wrote:
I looked and seem to be guilty of my own criticism in one or two
places my own code checking for _substrings_ of messageText; checking
for words I knew would be very probable for conveying that error.  I'm
not feeling compelled to "fix" it either, so, okay.  :)

>> So, of the 4 tests in this method, I think the second one should be
>> removed, and the last two should be made like the first one (e.g.,
>> without the string literals).
>
> I'm with you on the removal of duplication. Personal preference would
> be to say something like self assert: (e messageText
> includesSubString: '#newForMethod').

Yea, something that gives some flexibility over exact-wording, seems preferable.