Discussion, demonstrating error handling

[bencoman]

Tim & Sam, Seeking your opinions. Pharo has a powerful and flexible exception handling system that I don't feel we are demonstrating. As I've been working through the Bowling exercise, I've developed some concerns with this test pattern...

test15_RollsCannotScoreNegativePoints
    | result |
    self
        should: [ result := bowlingCalculator scoreRolling: -1 after: #() ]
        raise: BowlingError
        whoseDescriptionIncludes: 'Negative roll is invalid'
        description: 'Should get an error with the correct description'

because:

1. It doesn't teach students how errors to handle errors in application code. I feel the following teaches students more...

   test15_RollsCannotScoreNegativePoints
   | testError |
   [ bowlingCalculator scoreRolling: -1 after: #() ] 
       on: Error do: [ :error | testError := error ].
   self assert: testError notNil.  
   self assert: testError description equals: 'Negative roll is invalid'.  

2. ~Its using a facility not in the base image, so students can be confused when they try to copy the pattern in their own projects.~ [EDIT: Whoops, my presumption was wrong. It is in the base image]
3. The Error-instance is not available for inspection.
4. With my Bowling solution I want to demonstrate the use of custom errors by seeding the following with the downloaded exercise...

   
   Error subclass: #BowlingError
   instanceVariableNames: 'artifact description'
   classVariableNames: ''
   package: 'Exercism-Bowling'

BowlingError>>artifact: anObject artifact := anObject

BowlingError>>artifact ^ artifact

BowlingError>>description: errorString description := errorString

BowlingError>>description ^ 'Bowling error: ' , description

Frame>>raiseBowlingError: errorString artifact: anObject

"Raises a custom error with state holding the troublesome entity" (BowlingError new artifact: anObject; description: errorString) signal FrameTest>>test16_customError | error | [ Frame new raiseBowlingError: 'My custom error' artifact: 42] on: BowlingError do: [ :err | error := err ]. self assert: error notNil. self assert: (error description includesSubstring: 'My custom error'). self assert: error artifact equals: 42. FrameTest>>test17_negativeRollError | error | [ Frame new roll: -1 ] on: BowlingError do: [ :err | error := err ]. self assert: error notNil. self assert: (error description includesSubstring: 'Negative roll is invalid'). self assert: error artifact equals: -1 ``` and leave students to write... ``` Frame>>roll: anInteger anInteger < 0 ifTrue: [ self raiseBowlingError: 'Negative roll is invalid' artifact: anInteger ]. self isFull ifTrue: [ ^ self nextFrame roll: anInteger ]. frameRolls add: anInteger ```

[macta]

Hi Ben - you do find interesting things...

The current test pattern is of course auto-generated and generic - but better than the previous auto-generated one.

I see what you are getting at - and think you are on to something, but i'm not so keen on the 3 post asserts (the first one checks that an exception even happened).

I think we definitely could up the anti on being specific about students creating a specific exception (it was fine for an early exercise to use anything handy - but probably before bowling we should have an exercise specify an exercise specific error - so raise: MyError whoseDescr....:

Back to the second part - I think the use of higher order assertions is good (and hence my aversion to 3 asserts and a variable) - however I think you are correct that upping the anti on using a smarter exception is a good thing - so why don't we provide:

self should: [ result := bowlingCalculator scoreRolling: -1 after: #() ]
      raise: BowlingError
       specifying: [:ex |
                     self assert: ex message includes: 'my message'.
                     self assert: ex artefect equals: 42 ]

This is the neater way (note it also raises the issue that SUnit is backwards with beginsWith and includes support - raised recently when some collection extensions were added and fought for as CI was failing with undeciperhable messages). So I would add the beingsWith and Includes support so students get a clean message, and maybe we can get them added to SUnit. We could add stateSpec which has a nice dsl for all this, but it adds complexity.

[bencoman]

I like that. It covers points (3) & (4).
Actually just discovered there is already should:raise:withExceptionDo:.
Point (2) was incorrect. We need to do (1) at some point, but I'm happy for your pushback its not for every exercise.

[samWson]

I've been reading up on exceptions just last week. The book Deep into Pharo (Language -> Handling Exceptions) covers exceptions very well. The style you've shown @bencoman very much fits with what I read and I like your reasoning. I'm happy to go along with the conclusion you two have already come to.