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

Fri Feb 1 09:58:15 PST 2019

JDevlieghere added a comment.

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.

> - 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 :-)

> However, it's generally better to test the public interface of a method/class/entity, and in this case, I believe it should be possible.

It's 100% my fault for not making this clearer, but all that is in the pipeline. I wasn't able to work on this as much as I would've wanted the last week so that might have given the wrong impression. Maybe I should've kept the title as WIP, but I didn't want to discourage people from looking at the framework while I was adding this tests. Maybe I should've done this in a separate patch.

> I'm imagining some tests like having a dummy class with a bunch of methods which are annotated with the SB_RECORD macros. Then, in the test you call the methods of this class with some arguments, have the repro framework serialize the calls to a (in-memory?) stream, and verify the contents of that stream. This will test a lot more code  -- the (De)Serialize functions are just fancy ways of invoking memcpy, but if you take all of that together with the SB_RECORD, SB_REGISTER macros, it becomes some pretty deep magic that is interesting to test exhaustively (it's fine, but not really interesting to test that Deserialize<T*> and Deserialize<T&> do the right thing for each of the possible fundamental types.) And if you use the deserializer interface to read out the recorded traces (instead of comparing raw bytes), then you can avoid depending on the endianness of the values.
> 
> Conversely, you can write a trace file with the serializer interface, and then tell the replayer to invoke the mock class which will verify that it was called with the right arguments.

This is very much what I'm planning to do. Coming back to my original point about testing smaller units, this is all great while it works, but it's going to be far from obvious as soon as this breaks. My hope is that if something fundamental breaks, one of the "obvious" unit test would catch it first.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56322/new/

https://reviews.llvm.org/D56322