Fix variable toolview not sync with framestack view.
ClosedPublic

Authored by qi437103 on Apr 8 2016, 12:23 AM.

Details

Summary

The problem is caused by the active frame/thread in IVariableController,
which aren't properly updated when autoUpdate() == UpdateNone.

This commit removes m_active{Frame,Thread} because we already only call
update() in the event handler of frame/thread changed event.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
qi437103 updated this revision to Diff 3202.Apr 8 2016, 12:23 AM
qi437103 retitled this revision from to Fix variable toolview not sync with framestack view..
qi437103 updated this object.
qi437103 edited the test plan for this revision. (Show Details)
qi437103 added a reviewer: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptApr 8 2016, 12:23 AM
qi437103 updated this revision to Diff 3323.Apr 13 2016, 6:18 PM

A simpler patch that fixes the bug without removing m_active{Thread/Frame}

This is achieved by always update m_active{Thread/Frame} regardless of m_autoUpdate

apol added a subscriber: apol.Apr 13 2016, 10:27 PM

+1 maybe it would be interesting to get an automated test for these issues.

updated my answers.

@apol I'm new to this, how would you test a GUI?

apol added a comment.Apr 13 2016, 11:18 PM

For starters, this is not "a GUI", this is code (more specifically an API).

See how others test features in plugins, but basically it will be about creating a mock VariableController code.

@apol This is a bug triggered by certain status changes in the GUI (the variables list is visible or not). If I understand correctly, what you mean is to test only the code part of the issue (maybe by setting m_autoUpdate == None in test setup)?

apol added a comment.Apr 13 2016, 11:51 PM

Changes in the views will change the state of the instance. This is something you can test by using the API as well.

@apol Got it! It seems mock objects go in kdevplatform/tests, and actual unit test goes in kdevplatform/debugger/tests, right? I'm looking at kdevplatform/outputview/tests as an example.

mwolff added a subscriber: mwolff.Apr 24 2016, 1:20 PM

Yep, if your mock is generic, put it into kdevplatform/tests. Regarding the test, if you can make a generic test in kdevplatform - awesome. But maybe it's easier to add a test into kdevelop/debuggers/gdb that shows this issue and test against that then.

qi437103 updated this revision to Diff 3630.May 4 2016, 1:32 AM

Add unit tests

  • Add mock objects for IDebugSession related classes
  • Add unit tests for IVariableController

Not sure if I'm approaching the right way. It looks a little fragile to me to reproduce the bug like this (by multiple sequential state changes and checking update count) in the test. Any better ideas?

apol accepted this revision.May 4 2016, 11:02 AM
apol added a reviewer: apol.

Looks like a good start, I'm a bit afraid that we might be mocking too much, but we'll only get to the sweet spot by doing.

If you want some incentive, look at our coverage section in build.kde.org :)
https://build.kde.org/view/Kdevelop/job/kdevelop%205.0%20stable-kf5-qt5/16/PLATFORM=Linux,compiler=gcc/cobertura/

This revision is now accepted and ready to land.May 4 2016, 11:02 AM

Great! But it seems I can't find a detailed workflow description for phabricator. Is the next step to arc land fix-variable-sync-simple as shown on the top of this page? Or should I wait until all reviewers accept this fix?

apol added a comment.May 5 2016, 10:59 PM

I accepted it the last time. :)

qi437103 updated this revision to Diff 3651.May 5 2016, 11:34 PM
qi437103 edited edge metadata.

Change commit author to fullname otherwise it's not allowed to push to master

qi437103 updated this revision to Diff 3652.May 6 2016, 2:45 AM
  • Fix Bug 333759 Variables tool view not in sync with frame stack view.
  • Add mock objects for IDebugSession related classes
  • Add unit tests for IVariableController
This revision was automatically updated to reflect the committed changes.