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.

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

fixing the code update bug after compiling a missing method with the unfiltered stack #491

Conversation

adri09070
Copy link
Contributor

Fixes #489

When you encounter a DNU with the unfiltered stack in the debugger, the DNU context is at the top of your stack.

If you clicked the create "button" to create the missing method, the DebugSession will update the top context (here the DNU) with the new implemented method.

Then the event #contextChanged is triggered so that the debugger updates the stack, the code, the inspector, etc.

However, to update the code in StDebugger>>#updateCodeFromContext:, the debugger executes first its method #recordUnsavedCodeChanges that checks if there are unsaved code changes. To do that, the debugger checks if the displayed code is different from the code of the selected context. Here this is the case, because the code of the DNU is different from the code of the newly implemented method.

As the debugger considers there are unsaved code changes, it prevents from updating the code, which is wrong.

I fixed it so that the code is forced to update in the case of the top context has changed.

I set this PR as a draft because I would like to write a test

@adri09070 adri09070 marked this pull request as ready for review March 23, 2023 11:28
Copy link
Member

@StevenCostiou StevenCostiou left a comment

Choose a reason for hiding this comment

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

The change is good.
I have small doubts about the test, that could be completed to be more thorough.
But it can be discussed.

Comment on lines 418 to 423
"code must have been updated with the new method"
self
assert: dbg interruptedContext compiledCode selector
identicalTo: #foobar.
self assert: (dbg code text beginsWith: 'foobar')
]
Copy link
Member

Choose a reason for hiding this comment

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

Should we also test that the new current context method is the method foobar?

Copy link
Member

Choose a reason for hiding this comment

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

And maybe also that the correct text portion has been highlighted?

Copy link
Contributor Author

@adri09070 adri09070 Mar 24, 2023

Choose a reason for hiding this comment

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

Should we also test that the new current context method is the method foobar?

I've tested only the method selector and not the method itself because I cannot reference a method that doesn't exist. How else could I check that? Maybe with something like:

self assert: dbg interruptedContext compiledCode identicalTo: (StTestDebuggerProvider methodDictionary at: #foobar)

Like that, I could check the entire source code so it could be better?

And maybe also that the correct text portion has been highlighted?

Maybe we could test that but that's not what I wanted to test initially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@adri09070
Copy link
Contributor Author

really weird failing tests that are debugger-related.

I don't think that my changes introduced it though. Or maybe I am wrong?

@adri09070
Copy link
Contributor Author

The compilation of the method triggers a MethodAdded announcement that sends a message to a nil process in the debug session. That may be because of the way I initialize the debugger in my test?

@adri09070 adri09070 added the need more work This needs to be improved before being considered label Mar 29, 2023
@adri09070
Copy link
Contributor Author

adri09070 commented Mar 29, 2023

I think I understand.

This is related to the fact that we have 2 variables for debuggers in the debugger test class. Only the variable debugger is unsubscribed from the system announcer and I use the variable dbg

I left a comment in StDebuggerTest>>#tearDown:

"In tests, should we use debugger when we need a fully initialized debugger and dbg when we just need a debugger created with #basicNew? Still, isn't it weird to clear the session only for a fully initialized debugger but not for a debugger created with #basicNew? Both have a session. Maybe this implies that dbg  should only use the instance variable session and nothing else. These specifications should be written somewhere, in the doc of the class maybe."

@adri09070 adri09070 removed the need more work This needs to be improved before being considered label Mar 29, 2023
@StevenCostiou StevenCostiou merged commit 9c015fa into pharo-spec:Pharo11 Mar 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debugger: Creating an unexisting method does not refresh the view when stack is unfiltered
2 participants