[Lldb-commits] [PATCH] D39314: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D39314#908115, @sas wrote:

> In https://reviews.llvm.org/D39314#908065, @davide wrote:
>
> > can you please try adding a test for this one? :)
>
>
> To be fully honest, I'm not 100% sure how I'm supposed to add tests for this. I looked through the directories containing unit tests and didn't find anything specific to DYLD testing.
>
> I'm going to try to sync with @zturner to see if he is still able to run unit tests on Windows. We exclusively debug Windows remotes from a macOS or Linux host, so I don't have any setup to run local windows tests.


I understand. I'm not picking on you, of course, and I appreciate you trying to do that.
>From what I can tell, lack of testing can cause a lot of problems in the future [i.e. I could just remove part of your functions, and all the tests would pass anyway].
This is not ideal, from somebody who's trying to get more involved in lldb with a LLVM background :)
I think every change committed to the codebase should have a test associated, unless it's NFC.
Sorry if this sounds like captain obvious, but in case we can't test something, we might consider slowing down and revisiting the testing infrastructure that's there and improve it.
There's of course a tension between adding features and reducing technical debt, but keep checking fixes for new code without tests associated is a slippery road. Happy to discuss this further (and i know @zturner has ideas on how to fix this)


https://reviews.llvm.org/D39314