<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 29, 2016 at 9:48 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">We are talking about a very small cost here. Even before this change,<br>
the time of running this test is ~1s.<br></blockquote><div><br></div><div>Sure - but partly I'm arguing on principles here because the tests all add up over time. I'd like to make sure we're developing/sustaining good habits about what we add to the test/edit cycle of developers on the project.</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>
On Fri, Jan 29, 2016 at 9:38 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, Jan 29, 2016 at 9:29 AM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> It is the testing the related APIs for PGO profile reader.<br>
><br>
><br>
> Sure, but I'm asking why it's insufficient to test it just once/with a<br>
> smaller input - what value is added by testing it with such a large input<br>
> (why not 512? and if that, why not 1 or some otherwise trivially small<br>
> input?) or with 10 different levels of padding?<br>
><br>
> Unit tests in particular, and our regression suite in general, are generally<br>
> designed to be specifically targeted/purposeful tests. Broader input testing<br>
> would be left to the test-suite and/or fuzzing tests (especially coverage<br>
> based fuzzing which seems likely to provide better value per CPU cycle than<br>
> a more manual/repeated approach like this)<br>
><br>
>><br>
>><br>
>> David<br>
>><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<br>
>> > 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<br>
>> > 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<br>
>> > 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<br>
>> > that it<br>
>> > works") - maybe something better suited to ASan Fuzzing rather than a<br>
>> > unit<br>
>> > test.<br>
>> ><br>
>> > Blanket testing with broad inputs probably isn't suitable for a unit<br>
>> > test.<br>
>> ><br>
>> > A single test that demonstrates that the contents is smaller compressed<br>
>> > (if<br>
>> > there's a non-compressed option) might be worthwhile. Though it looks<br>
>> > like<br>
>> > this test doesn't even test that - I assume I could remove the<br>
>> > 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>><br>
>> >> wrote:<br>
>> >> > If we're asking the question - why does this need to be 1024? What's<br>
>> >> > the<br>
>> >> > specific purpose of the iteration? (why would 1 be too few<br>
>> >> > 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>
><br>
><br>
</div></div></blockquote></div><br></div></div>