Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modified StDebuggerActionModel to use Exception>>defaultDescription… #414

Open
wants to merge 5 commits into
base: Pharo13
Choose a base branch
from
Open

Modified StDebuggerActionModel to use Exception>>defaultDescription… #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b2a753
- Modified StDebuggerActionModel to use Exception>>defaultDescription…
emdonahue Sep 2, 2022
19f014f
Added tests for the debugger's printDescription on several different …
emdonahue Sep 7, 2022
a4e8527
- Made Halt print without context when signalled directly, to match e…
emdonahue Sep 7, 2022
1c1393b
Merge a4e8527dee99d435c25f013fc4e270db68c69e6a
emdonahue Sep 7, 2022
2e6dd93
Reasserting reverted description string
emdonahue Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/NewTools-Debugger-Tests/StDebuggerContextPredicateTest.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,28 @@ StDebuggerContextPredicateTest >> testIsSteppable [
self deny: predicate isSteppable
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintDNUDescription [
self assert: (StDebuggerActionModel on:
([ Object perform: #thisMessageIsNotUnderstood ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'Instance of Object class did not understand #thisMessageIsNotUnderstood'.
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintDescription [
self skip.
self assert: predicate printDescription equals: '0'
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintExceptionDescription [
self assert: (StDebuggerActionModel on:
([ Exception signal ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'Exception'.
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintHaltDescription [
|haltContext|
Expand All @@ -56,9 +72,41 @@ StDebuggerContextPredicateTest >> testPrintHaltDescription [
self assert: predicate printDescription equals: 'Halt in ', haltContext printString
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintHaltIfDescription [
self assert: (StDebuggerActionModel on:
([ Halt if: [ true ] ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'Halt in [ Halt if: [ true ] ] in StDebuggerContextPredicateTest>>testPrintHaltIfDescription'.
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintHaltNowDescription [
self assert: (StDebuggerActionModel on:
([ Halt now ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'Halt'.
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintPostMortemDescription [
self skip.
predicate postMortem: true.
self assert: predicate printDescription equals: '[Post-mortem] 0'
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintSignalInDescription [
self assert: (StDebuggerActionModel on:
([ Exception signalIn: thisContext ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'Exception'.
]

{ #category : #tests }
StDebuggerContextPredicateTest >> testPrintTestFailureDescription [
self assert: (StDebuggerActionModel on:
([ TestFailure signal ] on: Exception do:
[ :e | StTestDebuggerProvider new sessionFor: nil exception: e ]) session) statusStringForContext
equals: 'TestFailure'.
]
8 changes: 8 additions & 0 deletions src/NewTools-Debugger/Halt.extension.st
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
Extension { #name : #Halt }

{ #category : #'*NewTools-Debugger' }
Halt >> description [
(self signalContext receiver isKindOf: Exception)
& (self signalContext selector = #signal) ifTrue: [ ^ 'Halt' ].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is bad. #isKindOf: is costly and the behavior can be reproduced by calling StDebuggerContextPredicate>>#printDescriptionPrefixOn:. Using this method would also allow to handle post-mortems (which are just dead contexts: contexts that have already terminated in which we can't step anymore).

Overriding #description in the class Halt causes the debugger to display Halt when a breakpoint is hit, because the class Break inherits from Halt.

From looking at the code of StDebuggerActionModel>>#printHaltDescription:, I would say that the fact that "Halt in" not being displayed by the debugger when executing Halt now is a bug, so trying to reproduce it like that is not good.

^ 'Halt in ', self signalContext asString.
]
2 changes: 1 addition & 1 deletion src/NewTools-Debugger/StDebuggerActionModel.class.st
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ StDebuggerActionModel >> stackOfSize: anInteger [
{ #category : #context }
StDebuggerActionModel >> statusStringForContext [

^ self contextPredicate printDescription
^ self exception description
Copy link
Contributor

@adri09070 adri09070 Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't throw away context predicates like this.

For example, it handles printing for post-mortems and halts.
DoIts are also handled by context predicates. DoIts are just contexts with an OupsNullException and that's why the debugger doesn't display them correctly after your changes.

Plus, what you do is done in StDebuggerErrorContextPredicate: it prints the description of the exception only if it the context is not a DoIt.
So, if I look at the problem from the beginning, the problem is simply that an StDebuggerErrorContextPredicate is not created because the debugger action model doesn't consider exceptions signaled via #signalIn: as exceptions.
As you said, this is because of the method:

StDebuggerActionModel>>#contextSignalsAnException
  ...
  and: [  aContext selector = #signal ] ]

So, I would propose as a fix to:

  • remove Halt>>#description,
  • reverse StDebuggerActionModel>>#statusStringForContext to what it was before the changes,
  • in StDebuggerActionModel>>#contextSignalsAnException, change the line and: [ aContext selector = #signal ] ] to and: [ (#signal signalIn:) includes: aContext selector ] ]
  • Then, in StDebuggerErrorContextPredicate>># printDescription, we would have to handle Halt just the same way as OupsNullException. To avoid using kindOf, you could define an extension method in the class Exception isDebuggerWhiteListed that would return false and that would be overriden by OupsNullException and Halt to return true...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. I think what is needed most is a set of additional test cases that clarify some of the intended behavior. Since I don't have a spec for the exception handling system, I can only code against guesses about what correct behavior should look like based on examples I find in the image, some of which seem like bugs, as you mention, and some of which I could not find in order to produce, such as any code that generates a post-mortem. Could you perhaps suggest some tests that clarify some of these issues and intended behaviors?

Copy link
Contributor

@adri09070 adri09070 Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can add a test in which a debugger action model proceeds until it reaches a method with a Halt that uses #signal (such as 1 halt) and we could check that the description is the same as the one we would get with Halt if: [true]: "Halt in class>>method`

For post-mortems, it is really hard to get one and I can't think of an easy way to do it. Generally, it happens when we materialize a stack that had been previously fueled out in a file.
In a unit test, what you could do is create one "artificially". You could create a debugger action model on any session and store the context status string into a variable. Then, you could make the context post-mortem artificially by executing debuggerActionModel contextPredicate postMortem: true .
Then, you could check that the context status string is the same as the one you've stored into your variable but prefixed by [Post-mortem] .

I don't think there are more cases than these ones and the ones you already cover in your tests. I don't know what @StevenCostiou thinks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will try to get that working next time I have time to work on this. Would it be preferable to pull request just the tests, or is it preferred to have working code along with tests?

I'm trying to get my contracts/type checking library working, but it is considerably less useful than it could be if the exceptions it throws can't describe the type error.

]

{ #category : #'debug - stepping' }
Expand Down