On Tue, Oct 2, 2012 at 8:49 AM, Dmitri Gribenko <span dir="ltr"><<a href="mailto:gribozavr@gmail.com" target="_blank" class="cremed">gribozavr@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Oct 2, 2012 at 6:46 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com" class="cremed">alexfh@google.com</a>> wrote:<br>

> On Tue, Oct 2, 2012 at 5:36 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" class="cremed">gribozavr@gmail.com</a>> wrote:<br>
>><br>
>> On Tue, Oct 2, 2012 at 6:28 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com" class="cremed">alexfh@google.com</a>><br>
>> wrote:<br>
>> > On Tue, Oct 2, 2012 at 5:15 PM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" class="cremed">gribozavr@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Tue, Oct 2, 2012 at 5:59 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com" class="cremed">alexfh@google.com</a>><br>
>> >> wrote:<br>
>> >> > So I'm definitely for using FileCheck in this case, but it's only me,<br>
>> >> > others<br>
>> >> > may disagree.<br>
>> >><br>
>> >> I'm in favor of ASTMatchers-based tests for anything dumping related,<br>
>> >> based on my experience of maintaining<br>
>> >> test/Index/annotate-comments.cpp.  Any small change to the testcases<br>
>> >> forces a change of line numbers for everything that follows it (half<br>
>> >> of tests, on average).<br>
>> ><br>
>> > FileCheck has variables now, maybe we can add a special __LINE__<br>
>> > variable<br>
>> > (possibly with a way to specify offset)? In this case we could put CHECK<br>
>> > lines near the actual test code and this would not be bound to absolute<br>
>> > line<br>
>> > numbers. What do you think?<br>
>><br>
>> That would help somewhat.  But nevertheless, annotate-comments.cpp is<br>
>> the slowest testcase in the testsuite (for some reason, FileCheck<br>
>> takes up to 10s to match it in debug version -- spending most of the<br>
>> time in regex matching).<br>
><br>
><br>
> I'll look into adding special variables to FileCheck.<br>
><br>
> As for debug build, can we somehow use release FileCheck even when testing a<br>
> debug configuration?<br>
><br>
>><br>
>> >> With ASTMatchers-based tests we can keep all<br>
>> >> logically related tests in a single file and keep the test case close<br>
>> >> to the expected output while ensuring that different tests are<br>
>> >> isolated from each other.<br>
>> ><br>
>> > Isolation is possible now using separate RUN lines with specific -D<br>
>> > defines<br>
>> > and #ifdef blocks.<br>
>><br>
>> That's more like putting the isolation into a tool that didn't have<br>
>> it, versus having an approach where testcases are intrinsically<br>
>> isolated.<br>
><br>
> My problem with ASTMatchers-based tests for dumping is that there are lots<br>
> of dependencies, that can be avoided. And the test infrastructure (a custom<br>
> test infrastructure, used only for one test) is an additional possible point<br>
> of failure, and definitely requires some support. In my opinion it makes<br>
> sense only in case this infrastructure is generalized, moved elsewhere and<br>
> gets its own tests. But still this seems to be overkill.<br>
<br>
</div></div>The infrastructure is rather simple in my opinion, and can (should!)<br>
be shared between (Decl|Stmt)(Printer|Dumper) tests.</blockquote><div><br></div><div>So, generally I do agree. However, FileCheck was *designed* for testing textual output. It seems a bit weird to not be using it here...</div>
<div><br></div><div>I wonder if it would make sense to hoist the FileCheck logic into a library such that the unittests could actually use the same syntax for making textual assertions from within unittests? This has been my biggest frustration with unittests checking textual output -- often the errors do not make it clear what broke or how to fix. They also tend to devolve to golden-output tests rather than using minimal assertions to check for the important output.</div>
</div></div>