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