[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

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