<div dir="ltr">Ping</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 1, 2016 at 6:22 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><span class="">
<br>
<br>
<div>On 01/29/2016 10:01 AM, David Blaikie
via llvm-commits wrote:<br>
</div>
<blockquote type="cite">
<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"></a><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>
</div>
</blockquote></span>
+1. <br><div><div class="h5">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><font color="#888888"><br>
David<br>
</font></span>
<div>
<div><br>
On Fri, Jan 29, 2016 at 9:38 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank"></a><a href="mailto:dblaikie@gmail.com" target="_blank">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" target="_blank">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" target="_blank">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" target="_blank">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" target="_blank">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" target="_blank">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" target="_blank">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>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div>