r356569 - [NFC][clang][astdump] Some baseline tests for OpenMP

[Richard Smith via cfe-commits]

On Fri, 31 May 2019 at 16:20, Roman Lebedev via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Sat, Jun 1, 2019 at 2:00 AM Richard Smith <richard at metafoo.co.uk>
> wrote:
> >
> > These are change detector tests (
> https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html)
> and create a significant maintenance burden that exceeds their value.
> >
> > 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.
> Yep, lack of tooling indeed always makes things rather frustrating.
> I have recently highlighted that in yet another AST thread:
> https://lists.llvm.org/pipermail/cfe-dev/2019-April/061997.html
>
> I personally added those tests to highlight the openmp_structured_block
> changes, but there is also unit-test coverage for that.
>
> If maintaining test coverage turns out to be too problematic,
> feel free to degrade the test coverage as you see fit,
> but i stand by my opinion that it is a preexisting tooling issue.
> (same with clang codegen tests, diags, etc etc)
>

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

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.

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.

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.


> Roman.
>
> > On Wed, 20 Mar 2019 at 11:16, Roman Lebedev via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
> >>
> >> Author: lebedevri
> >> Date: Wed Mar 20 09:31:47 2019
> >> New Revision: 356569
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=356569&view=rev
> >> Log:
> >> [NFC][clang][astdump] Some baseline tests for OpenMP
> >>
> >> Summary:
> >> Split off from D59214.
> >> Not a fully exhaustive test coverage, but better than what there
> currently is.
> >>
> >> Differential Revision: https://reviews.llvm.org/D59306
> >>
> >> Added:
> >>     cfe/trunk/test/AST/ast-dump-openmp-atomic.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-barrier.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-cancel.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-cancellation-point.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-critical.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-distribute-parallel-for-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-distribute-parallel-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-distribute-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-distribute.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-flush.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-for-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-master.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-ordered.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-parallel-for-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-parallel-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-parallel-master-XFAIL.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-parallel-sections.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-parallel.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-section.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-sections.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-single.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-data.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-enter-data.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-exit-data.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-parallel-for-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-parallel-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-parallel.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-simd.c
> >>
>  cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for-simd.c
> >>
>  cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-parallel-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-teams-distribute.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-teams.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target-update.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-target.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-task.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-taskgroup.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-taskloop-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-taskloop.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-taskwait.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-taskyield.c
> >>
>  cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-parallel-for-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-parallel-for.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-teams-distribute-simd.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-teams-distribute.c
> >>     cfe/trunk/test/AST/ast-dump-openmp-teams.c
> >>
> >...
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190603/a6319a8d/attachment-0001.html>