<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 8, 2016 at 4:38 PM, David Blaikie via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Feb 8, 2016 at 4:25 PM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Mon, Feb 8, 2016 at 4:12 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 8, 2016 at 4:00 PM, Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> Yes, if we talk about all tests in different repos collectively, 0 is<br>
>> probably covered -- but that is still an assumption from the<br>
>> unittest's point of view.<br>
><br>
><br>
> Ah, sure - I agree it (along with any other API surface area) should be<br>
> covered by a unit test, even if the codepath is also covered by a higher<br>
> level test of some kind.<br>
><br>
> But it looks like the 0 padding case is covered by other /unit/ tests in the<br>
> same file.<br>
<br>
</span>AFAIK, this is the only unittest that covers 0 padding case.<br></blockquote><div><br></div></span><div>Presumably any other test testing InstrProfSymtab::create tests the zero padding case, so far as I'm understanding? (instr_prof_symtab_test, instr_prof_symtab_module_test?)<br><br>(also, as I look at this - the tests seem generally pretty repetitive and brittle - repeating the same string in many places, for example & seem overly verbose, testing several more inputs than seem necessary/constructive (instr_prof_symtab_test tests 5 strings - why not one or two? instr_prof_symtab_module_test tests even more... - no tests seem to test a failed lookup (getFuncName with a hash that is not present))<br><br></div></div></div></div></blockquote><div><br></div><div>No -- those tests do not test serialization nor testing combining two serial segments together which is tested here (simulating what linker does).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>It looks like this could be simplified across the board & make it clearer what's being tested and how it's covering all the functionality of the InstrProfRecord class. </div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><br>
> This seems like a reasonable case of test case reduction by<br>
> combination (if you have {a, b} x {x, y} to test you can test it in two<br>
> cases {a, x}, {b, y} rather than running 4 tests, since the features are<br>
> independent).<br>
<br>
</span>We need to be very careful about reducing {a,b}x{x,y} into {a,x} and<br>
{b,y} though -- it makes explicit assumption about the implementation<br>
-- there might be subtle dependencies there..<br></blockquote></span><div><br>Agreed, though it is impractical to assume everything's related to everything else - testing would be prohibitive. So we generally test fairly precisely/narrowly within the regression suite, and leave it to integration tests (like the test-suite, self hosts, etc) to test more broadly for the cross-functional interactions.<br><br>That's not to say that some tests don't end up a bit redundant so as to get more explicit coverage.<br></div></div></div></div></blockquote><div><br></div><div>yes -- that is why I said 'we need to be careful' -- one of the biggest pitfalls of writing tests is making assumptions above coverage.  In lack of strong proof, I would err on the more conservative side.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>- David<br> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>
David<br>
</font></span><div><div><br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>> On Mon, Feb 8, 2016 at 3:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Feb 8, 2016 at 3:53 PM, Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Mon, Feb 8, 2016 at 3:30 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Feb 8, 2016 at 3:17 PM, Xinliang David Li<br>
>> >> > <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> On Mon, Feb 8, 2016 at 3:12 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>> >> >> wrote:<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > On Mon, Feb 8, 2016 at 3:08 PM, Xinliang David Li<br>
>> >> >> > <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> On Mon, Feb 1, 2016 at 10:44 AM, David Blaikie<br>
>> >> >> >> <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
>> >> >> >> wrote:<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > On Fri, Jan 29, 2016 at 5:58 PM, Xinliang David Li<br>
>> >> >> >> > <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>><br>
>> >> >> >> > wrote:<br>
>> >> >> >> >><br>
>> >> >> >> >> To clarify, it is not 128 iterations, but creating a symbol<br>
>> >> >> >> >> table<br>
>> >> >> >> >> with<br>
>> >> >> >> >> 128 entries -- which is a reasonable size.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > We don't generally test on "realistic" sized inputs in the<br>
>> >> >> >> > regression<br>
>> >> >> >> > suite.<br>
>> >> >> >> > We write targeted tests for functionality. Broad testing is<br>
>> >> >> >> > done<br>
>> >> >> >> > in<br>
>> >> >> >> > the<br>
>> >> >> >> > test-suite and other integration level testing.<br>
>> >> >> >> ><br>
>> >> >> >> >><br>
>> >> >> >> >> Test coverage wise, it is probably the same as a 3-entry<br>
>> >> >> >> >> symtab.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > Then let's use a 3-entry symtab.<br>
>> >> >> >> ><br>
>> >> >> >> > (why 3? Because it tests the boundaries (first and last) and<br>
>> >> >> >> > one<br>
>> >> >> >> > "normal"<br>
>> >> >> >> > case of a non-boundary value - while the boundaries probably<br>
>> >> >> >> > aren't<br>
>> >> >> >> > interesting in this algorithm, it's cheap enough to just follow<br>
>> >> >> >> > that<br>
>> >> >> >> > common<br>
>> >> >> >> > practice in test case design)<br>
>> >> >> >><br>
>> >> >> >> Will update it to 3.<br>
>> >> >> >><br>
>> >> >> >> ><br>
>> >> >> >> > I'm also curious about the padding parameter - what does it do?<br>
>> >> >> >> > Choose<br>
>> >> >> >> > how<br>
>> >> >> >> > many null characters go between each value? What effect does<br>
>> >> >> >> > that<br>
>> >> >> >> > have/why<br>
>> >> >> >> > is that a tuning parameter? (understanding what it's for can<br>
>> >> >> >> > help<br>
>> >> >> >> > us<br>
>> >> >> >> > choose<br>
>> >> >> >> > appropriate test cases/coverage for that functionality)<br>
>> >> >> >><br>
>> >> >> >> Internal padding bytes (for alignment to 4 bytes) can be zero to<br>
>> >> >> >> 3.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > Any idea what's particularly useful to test here? (does it just<br>
>> >> >> > assert<br>
>> >> >> > that<br>
>> >> >> > the parameter is [0,3] ? Or does it have well defined behavior<br>
>> >> >> > (returning an<br>
>> >> >> > error code? doing something else?) outside that range? is any case<br>
>> >> >> > more<br>
>> >> >> > interesting than any other - or just a simple loop for [0,Padding]<br>
>> >> >> > done<br>
>> >> >> > at<br>
>> >> >> > some point in the algorithm? Does anything test that the algorithm<br>
>> >> >> > emitted<br>
>> >> >> > the right padding?)<br>
>> >> >><br>
>> >> >> It tests that the reader is (flexible) and capable of handing<br>
>> >> >> padding<br>
>> >> >> bytes not produced by the writer.  How many paddings should be<br>
>> >> >> emitted<br>
>> >> >> is not specified. For instance, if some producer forces 8 byte<br>
>> >> >> alignment, it should be handled too.<br>
>> >> ><br>
>> >> ><br>
>> >> > Ah, OK - perhaps we could just test one pseudo-random (if it's really<br>
>> >> > just a<br>
>> >> > "while (null byte)" loop to ignore the padding - I'd probably pick 2<br>
>> >> > bytes<br>
>> >> > of padding, but don't mind any small number) amount of padding to<br>
>> >> > test<br>
>> >> > that<br>
>> >> > the reader ignores it, rather than testing several amounts of<br>
>> >> > padding?<br>
>> >> > Alternatively/in addition, might be good to test these features<br>
>> >> > separately<br>
>> >> > to make triage easier? Rather than combining compression and padding<br>
>> >> > together - unless there's an interesting interaction between the two<br>
>> >> > features in the implementation?<br>
>> >> ><br>
>> >><br>
>> >> I think 0 is more special here, so I would pick 0 and 1 byte.<br>
>> ><br>
>> ><br>
>> > Is zero bytes of padding not already covered by any other tests? (I<br>
>> > assume<br>
>> > it's covered by most tests as it sounds like it's the common case?)<br>
>> ><br>
>> >><br>
>> >> > You say "padding bytes not produced by the writer" - does the writer<br>
>> >> > produce<br>
>> >> > zero bytes of padding, or some amount of padding that's just not the<br>
>> >> > same<br>
>> >> > amounts as are being tested here?<br>
>> >><br>
>> >> The writer can produce 0 or more padding bytes, the assembler and<br>
>> >> linker may or may not pad more. The purpose of the testing is that the<br>
>> >> reader does not depend/care about those behavior.<br>
>> >><br>
>> >> David<br>
>> >><br>
>> >> ><br>
>> >> > - David<br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
<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></blockquote></div><br></div></div>