Ignore 'this' by checking the name of the variable #181
Conversation
|
@SarikaSinha, @iloveeclipse, Could you please have a look at this? Please let me know if this doesn't make sense or requires more changes. |
I will be able to look at this not before next week. |
|
Can you add some test like : |
|
@SarikaSinha, sure, I can add the required tests. Though, what should the new tests cover? The tests you mentioned along with |
It should cover the case of failure with the old code, which you have stated in the problem. And it should pass after the change ideally. |
|
Okay, that way.. I believe the issue only repros in with the dex files compiled for Android. How should we go about writing tests for that? We'd need to compile to dex/APK first and then likely run Android Emulator to test? |
|
Yes, something like that. |
|
Do we have any tests running on Android? If not, adding the entire infra to run tests on Android would be a fairly complex work, especially for someone like me who's new to the project. While I completely agree with the fact that we should add tests for this, I don't really have enough context in this project yet. I'd be willing to take a look, but would require some more help in figuring out how to go about this. |
|
We don't have tests for Android. Is it possible to get the compiled classes from Android and just execute with the scenario? |
|
I have attempted one POC for running tests on Android in #187. Have a look? Android SDK provides the tool for compiling to a dex file, which runs on the dalvik vm. The Dalvik VM itself seems to be officially supported only on Android. There seems to be some ports available to run Dalvik directly on a linux machine and such, but could not find anything fully convincing. We can do some more research, if that works, makes the life much simpler. In any case, if the current PR does not break any existing tests, can we go ahead with it and continue the discussion on the Android tests separately? |
|
Will release after 4.27 is complete. |
|
@SarikaSinha, I believe 4.27 release should be complete now? Can we merge this now to make sure this goes into the next release? |
There was a problem hiding this comment.
Just few formal recommendations for your PR
- Please create dedicated branch for your changes and do not commit on master branch directly.
- Please don't merge master brsnch into your branch to get latest master state
- Please fetch from master and rebase your branch on latest master state
- Please provide commit message that explains the fix and refers to original issue Missing variables when debugging Android apps #179
Fix eclipse-jdt#179, Missing variables when debugging Android apps - While getting the list of variables for a method, we were skipping the first slot assuming it was a reference to this. - Though that is not always the case when testing with Android dex files. - Instead of assuming `this` is at the first slot, check the name of the variable.
|
@iloveeclipse, Thanks for your suggestions. Yes, I agree having a dedicated branch helps, but changing it now would likely mean I'll have to create a new PR, losing the context. And I believe it would not matter much once merged? I've taken in the other three recommendations and updated the branch, please have a look? |
It will. Please create new PR from dedicated branch. The context is #179 and it will not be lost. |
|
Created #226. Closing this. |
What it does
Fix #179.
While getting the list of variables for a method, we were skipping the first slot assuming it was a reference to
this. Though this is not always the case when testing with Android dex files. Instead of assumingthisis at the first slot, check the name of the variable.How to test
Tested locally, the issue that was reported earlier no longer occurs.
Author checklist