-
Notifications
You must be signed in to change notification settings - Fork 99
Dealing with Unity dsyms and integration tests #26
base: master
Are you sure you want to change the base?
Conversation
phuesler
commented
Jan 29, 2015
- Always read the name from the symbol table
- Always use the first symbol if there are several symbols with the same address
- Add integration tests
- The integration tests show a bug with the cache. Without the cache disabled, they fail. With cache enabled they fail on first run and success on the 2nd run
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Thanks a lot! Conceptually these changes look good. Can you clean them up a bit and then I'll do a more thorough review? |
I have cleaned up the code. As fare as I can tell, the commented out code was only in a specific commit and not in the final diff of the pull request. I also refactored the tests with the intent to clarify what they are actually testing. The last test that I added shows, that on empty cache but with caching activated, the first run does not return a symbol but the second one does. |
Sorry for taking so long to get back to you, I will try to look over these today. |
Ok mostly just style things, otherwise it seems ok. Normally I'd have you create a clean set of diffs, it's still pretty hard to review with things like commented out code that get removed later and lots of whitespace changes. But it's a pretty big stack of diffs so if you don't want to go back and clean it up I'm ok with it for this round. The integration tests are really handy, I'll try to add some more at some point. Thanks for your hard work! |
Let's do this properly. I will redo the pull request so that the diff is easier to read and I'll clean up the formatting as well. I should have something ready by the end of next week. Feel free do close this pull request. |
Ok great, thanks. You can just push it to the same pull request branch if you want. |
* Always read the name from the symbol table * Always use the first symbol if there are several symbols with the same * address * Add integration tests * The integration tests show a bug with the cache. With cache disabled, they fail. With cache enabled they fail on first run and succeed on the 2nd run
I redid the pull request and squashed everything into one commit. Do you have any pointers why atosl returns different results with a disabled cache? |
Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed. |