JDevlieghere added a comment. In D56322#1381254 <https://reviews.llvm.org/D56322#1381254>, @labath wrote:
> In D56322#1380996 <https://reviews.llvm.org/D56322#1380996>, @JDevlieghere > wrote: > > > In D56322#1380442 <https://reviews.llvm.org/D56322#1380442>, @labath wrote: > > > > > I have a lot of comments. The two major ones are: > > > > > > - i think the way you link the tests is in the UB territory. I explain > > > this in detail in one of the inline comments. > > > > > > Thanks, you're right and your suggestion makes sense. Keep in mind that the > > registry's `Init` method needs to have access to the SB definitions though. > > > Yes, in this world, the I guess the version of the registry in the utility > folder would be just an abstract class which provides the building blocks to > intercept "any" api. And the API folder would contain an instantiation of > this class which intercepts the SB methods. (And tests would > instantiate/subclass/whatever it to intercept the mock methods.) > > > > > > >> - I believe that your unit tests (not just in this patch) focus too much > >> on testing the behavior of a single method, even when that method does not > >> have an easily observable results. For this you then often need to expose > >> internals of the tested class to be able to test some effect of that > >> method on the internal state. This not all bad (i've done it myself > >> sometimes), and it's definitely better that not writing any unit tests. > > > > It's funny you mention this, maybe I misremember but I recall you > > commenting on a patch that the current reproducers test were too high > > level. Maybe we have different views on what unit tests are, but I strongly > > believe that it's import to tests small *units* of code. They may seem > > trivial today until someone to decides to refactor them tomorrow. Indeed, > > we're already planning to move some of this code around and these tests > > will give me a lot more confidence. This stuff is relatively tricky, hard > > to debug and easy to get wrong. I strongly believe it's important to have > > these kind of unit tests, but I also totally agree that they should not be > > the only tests. See my next comment for more on that :-) > > Yes, that is funny, and a bit ironic. :) Though I think we're on the same > page here. My comment was more about the lack of higher-level (or, shall we > call them middle-level?) tests, then the presence of these low-level ones. > Lldb has a very big deficiency in low-level tests, so I'd rather have more > (even if some end up being "change-detector tests") than less (and miss some > opportunities to increase coverage) of them. Great :-) > Bottom line: if you write some tests like the one I described previously, I > am going to be _very_ happy. I have no problem with the existing tests > staying, if you find them valuable. Just please make sure they work on > big-endian platforms too. :) Awesome, yeah I didn't consider the float encoding, I'll just do a roundtrip test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
