<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 29, 2016 at 9:29 AM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It is the testing the related APIs for PGO profile reader.<br></blockquote><div><br></div><div>Sure, but I'm asking why it's insufficient to test it just once/with a smaller input - what value is added by testing it with such a large input (why not 512? and if that, why not 1 or some otherwise trivially small input?) or with 10 different levels of padding?<br><br>Unit tests in particular, and our regression suite in general, are generally designed to be specifically targeted/purposeful tests. Broader input testing would be left to the test-suite and/or fuzzing tests (especially coverage based fuzzing which seems likely to provide better value per CPU cycle than a more manual/repeated approach like this)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On Fri, Jan 29, 2016 at 9:24 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, Jan 29, 2016 at 9:15 AM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> The iteration is also encoded in the name.<br>
><br>
><br>
> That it tests compression? If we're using a standard compression algorithm,<br>
> I would expect we would just test the compression algorithm directly. I<br>
> don't think there's any need to test that the compression algorithm works in<br>
> any particular use case (test that the contents is compressed).<br>
><br>
> The input seems pretty arbitrarily chosen - it's not clear why that provides<br>
> the right tradeoff between time and coverage, it looks a bit scattershot<br>
> ("if we put enough stuff through this, perhaps it'll give confidence that it<br>
> works") - maybe something better suited to ASan Fuzzing rather than a unit<br>
> test.<br>
><br>
> Blanket testing with broad inputs probably isn't suitable for a unit test.<br>
><br>
> A single test that demonstrates that the contents is smaller compressed (if<br>
> there's a non-compressed option) might be worthwhile. Though it looks like<br>
> this test doesn't even test that - I assume I could remove the compression<br>
> support from this library & still pass the test...<br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>> On Fri, Jan 29, 2016 at 9:03 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> > If we're asking the question - why does this need to be 1024? What's the<br>
>> > specific purpose of the iteration? (why would 1 be too few iterations,<br>
>> > for<br>
>> > example?)<br>
>> ><br>
>> > On Fri, Jan 29, 2016 at 9:01 AM, David Li via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> davidxl accepted this revision.<br>
>> >> davidxl added a comment.<br>
>> >> This revision is now accepted and ready to land.<br>
>> >><br>
>> >> lgtm<br>
>> >><br>
>> >><br>
>> >> <a href="http://reviews.llvm.org/D16726" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16726</a><br>
>> >><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>