<div dir="ltr"><div dir="ltr">On Fri, 31 May 2019 at 16:20, Roman Lebedev via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, Jun 1, 2019 at 2:00 AM Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
><br>
> These are change detector tests (<a href="https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html" rel="noreferrer" target="_blank">https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html</a>) and create a significant maintenance burden that exceeds their value.<br>
><br>
> Please either reduce these to tests that actually test specific salient properties (rather than just being assertions that the AST does not change) or remove them.<br>
Yep, lack of tooling indeed always makes things rather frustrating.<br>
I have recently highlighted that in yet another AST thread:<br>
<a href="https://lists.llvm.org/pipermail/cfe-dev/2019-April/061997.html" rel="noreferrer" target="_blank">https://lists.llvm.org/pipermail/cfe-dev/2019-April/061997.html</a><br>
<br>
I personally added those tests to highlight the openmp_structured_block<br>
changes, but there is also unit-test coverage for that.<br>
<br>
If maintaining test coverage turns out to be too problematic,<br>
feel free to degrade the test coverage as you see fit,<br>
but i stand by my opinion that it is a preexisting tooling issue.<br>
(same with clang codegen tests, diags, etc etc)<br></blockquote><div><br></div><div>I disagree that this is a tooling issue. (I've added a script utils/make-ast-dump-check.sh to improve the maintainability issue for these tests for now, but they're still bad tests and should be fixed or removed.)</div><div><br></div><div>One major purpose of having tests is to allow us to change the code with confidence: if all tests pass, then the code change is probably correct, and if any test fails, the code change is probably wrong. For that to hold, the goal of a test should be to never fail when given a correct change, and to fail on some kinds of incorrect changes. (And then the coverage criterion is that at least one test fails for any incorrect change: clearly that's impossible as a goal, but it's an aspirational target.) With that in mind, change-detector tests like this are simply bad tests: they fail if the code is *changed*, regardless of whether the change is correct or not.</div><div><br></div><div>Note in particular that I'm pretty sure that no-one has validated that the test expectations in these tests are actually correct, only that they happen to match what Clang did at the point in time when they were created. I've found bugs in our OpenMP AST representation, and these tests are checking to make sure those bugs don't get fixed.</div><div><br></div><div>Also, now that we have a "regenerate the test expectations" script (which is really the only way to keep this kind of test manageable), we have another kind of problem: someone making an AST change that affects these changes will be inundated with pages and pages of test failures from these tests, and will likely use the script to make them go away after only validating the first few. If there are also bugs being found by these tests, those bugs will be hidden by the spam or generally ignored by people frustrated by these bad tests.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Roman.<br>
<br>
> On Wed, 20 Mar 2019 at 11:16, Roman Lebedev via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: lebedevri<br>
>> Date: Wed Mar 20 09:31:47 2019<br>
>> New Revision: 356569<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=356569&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=356569&view=rev</a><br>
>> Log:<br>
>> [NFC][clang][astdump] Some baseline tests for OpenMP<br>
>><br>
>> Summary:<br>
>> Split off from D59214.<br>
>> Not a fully exhaustive test coverage, but better than what there currently is.<br>
>><br>
>> Differential Revision: <a href="https://reviews.llvm.org/D59306" rel="noreferrer" target="_blank">https://reviews.llvm.org/D59306</a><br>
>><br>
>> Added:<br>
>> cfe/trunk/test/AST/ast-dump-openmp-atomic.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-barrier.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-cancel.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-cancellation-point.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-critical.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-distribute-parallel-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-distribute-parallel-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-distribute-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-distribute.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-flush.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-master.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-ordered.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-parallel-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-parallel-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-parallel-master-XFAIL.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-parallel-sections.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-parallel.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-section.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-sections.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-single.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-data.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-enter-data.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-exit-data.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-parallel-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-parallel-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-parallel.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-teams.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target-update.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-target.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-task.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-taskgroup.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-taskloop-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-taskloop.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-taskwait.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-taskyield.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-parallel-for-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-parallel-for.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-simd.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-teams-distribute.c<br>
>> cfe/trunk/test/AST/ast-dump-openmp-teams.c<br>
>><br>
>...<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>