<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 3, 2015 at 1:41 AM, Pavel Labath <span dir="ltr"><<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There is already code that enforces unique names (see dotest.py:1255).<br></blockquote><div><br></div><div>How long ago was that added? I literally hit this within the last 6-8 weeks on something internal.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I added this a while back because we were getting test flakyness<br>
because of that. I don't remember the exact reason but I think it had<br>
something to do with the log file names clashes, as all logs are<br>
placed in the same folder.<br>
<br>
I agree that it is possible to have multiple test files with the same<br>
name, and still have the names meaningful, but then we need to make<br>
sure that the it actually works reliably.<br>
<br></blockquote><div><br></div><div>I really have no problem with this being forced. With regards to other repos, this is no different than using a shared library where a base class may (in the future) add a method name that perturbs a downstream user (subclass). Removal of confusion trumps inconvenience in this case. I'm sold.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
As for the results formatter, I like the new output a lot.</blockquote><div><br></div><div>Glad to hear that. I'm looking forward to making it better.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> For me, the<br>
inability to recognize crashes is a blocker for using it, but once<br>
that works, I would definitely be in favor of making the default.<br>
<br></blockquote><div><br></div><div>Yep, that and timeouts. For us, we catch both on Jenkins and the Xunit formatter only because the Xunit results gatherer also checks the output, where the legacy summary support displays this information. (I have the new summary output wiring in the timeout data, but not the exceptional exit (i.e. signal-based exit)) bits.</div><div><br></div><div>I'll be addressing both of those in the extremely near future.</div><div><br></div><div>I'll be depending on the test event infrastructure's ability to handle re-runs to do the low-load, single-worker test pass, so I'm working on this test summary bit first. (And it's nice to have!).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would welcome shortening of the test names, but that is of secondary<br>
significance for me.<br>
<br></blockquote><div><br></div><div>Ultimately I think it's fair to say "hey just put one test case class in a file", and with a "test file base names must be unique" policy, I think we can even get away with "test_method_name (relative filename)" to identify the test method. That should be even shorter than what Zachary and I were discussing last night.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
great job,<br>
pl<br>
<br></blockquote><div><br></div><div>Thanks for the feedback!</div><div><br></div><div>Hopefully this serves you as well, Dawn?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 3 December 2015 at 06:20, Todd Fiala via lldb-dev<br>
<div class="HOEnZb"><div class="h5"><<a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>> wrote:<br>
><br>
><br>
> On Wed, Dec 2, 2015 at 9:48 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Wed, Dec 2, 2015 at 9:44 PM Todd Fiala <<a href="mailto:todd.fiala@gmail.com">todd.fiala@gmail.com</a>> wrote:<br>
>>><br>
>>><br>
>>>><br>
>>>> and the classname could be dropped (there's only one class per file<br>
>>>> anyway, so the classname is just wasted space)<br>
>>><br>
>>><br>
>>> Part of the reason I included that is I've hit several times where copy<br>
>>> and paste errors lead to the same class name, method name or even file name<br>
>>> being used for a test. I think, though, that most of those are addressed by<br>
>>> having the path (relative is fine) to the python test file. I think we can<br>
>>> probably get by with classname.methodname (relative test path). (From your<br>
>>> other email, I think you nuke the classname and keep the module name, but<br>
>>> I'd probably do the reverse, keeping the class name and getting rid of the<br>
>>> module name since it can be derived from the filename).<br>
>><br>
>> I don't think the filename can be the same anymore, as things will break<br>
>> if two filenames are the same.<br>
><br>
><br>
> Maybe, but that wasn't my experience as of fairly recently. When tracking<br>
> failures sometime within the last month, I tracked something down in a<br>
> downstream branch with two same-named files that (with the legacy output)<br>
> made it hard to track down what was actually failing given the limited info<br>
> of the legacy test summary output. Maybe that has changed since then, but<br>
> I'm not aware of anything that would have prohibited that.<br>
><br>
>><br>
>> We could go one step further and enforce this in the part where it scans<br>
>> for all the tests.<br>
><br>
><br>
> I think I can come up with a valid counterargument to doing that. I could<br>
> imagine some python .py files being organized hierarchically, where some of<br>
> the context of what is being tested clearly comes from the directory<br>
> structure.<br>
><br>
> Something like (I'm making this up):<br>
><br>
> lang/c/const/TestConst.py<br>
> lang/c++/const/TestConst.py<br>
><br>
> where it seems totally reasonable to me to have things testing const support<br>
> (in this example) but being very different things for C and C++, being<br>
> totally uniqued by path rather than the .py file. I'd prefer not to require<br>
> something like this to say:<br>
> lang/c/const/TestConstC.py<br>
> lang/c++/const/TestConstC++.py<br>
><br>
> as it is redundant (at least via the path hierarchy).<br>
><br>
> The other reason I could see avoiding that<br>
> unique-test-basenames-across-test-suite restriction is that it can become<br>
> somewhat of an unnecessary burden on downstream branches. Imagine somebody<br>
> has a branch and has a test that happens to be running fine, then somebody<br>
> in <a href="http://llvm.org" rel="noreferrer" target="_blank">llvm.org</a> lldb adds a test with the same name. Downstream breaks. We<br>
> could choose to not care about that, but given that a lot of our tests will<br>
> revolve around language features accessed/provided by the debugger, and a<br>
> number of language features pull out of a limited set of feature names (e.g.<br>
> const above), I could see us sometimes hitting this.<br>
><br>
> Just one take on it. I'm not particularly wedded to it (I probably would<br>
> avoid the confusion by doing something exactly like what I said above with<br>
> regards to tacking on the language to the test name), but I have hit this in<br>
> similar form across different language tests.<br>
><br>
>><br>
>> If it finds two test files with the same name we could just generate an<br>
>> error. I think that's a good idea anyway, because if two test files have<br>
>> the same name, then the tests inside must be similar enough to warrant<br>
>> merging them into the same file.<br>
><br>
><br>
> Maybe, but not in the real cases I saw across different languages. I think<br>
> for other areas of the debugger, this isn't an issue. So maybe language<br>
> feature tests just have to know to append their language (whether it be C,<br>
> C++, ObjC, etc.)<br>
><br>
>><br>
>><br>
>> If no two filenames are the same, and if there's only 1 class per file,<br>
>> then filename + method name should uniquely identify a single test, and so<br>
>> you could omit the class name and show a relative path to the filename.<br>
><br>
><br>
> I think we currently have some tests that have multiple test classes in the<br>
> test file. We could certainly verify that in TOT, and we could certainly<br>
> undo that which seems reasonable.<br>
><br>
> I'd be interested in what other people think here on restricting test names<br>
> to be unique across the repo. I could be convinced either way on allowing<br>
> two tests with the same name, but I'd probably avoid layering on a<br>
> restriction if it is entirely artificial and requires longer test names that<br>
> are otherwise uniqued by path.<br>
><br>
> Thanks for the feedback!<br>
> --<br>
> -Todd<br>
><br>
</div></div><div class="HOEnZb"><div class="h5">> _______________________________________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">-Todd</div></div>
</div></div>