On Wed, Jul 29, 2020 at 09:40:35AM +0100, Richard Sandiford wrote:
> Thanks for doing this. +1 for the best fix being to add XFAILing tests
> to the main testsute, enabled by default. I don't see any other realistic
> way of ensuring that fixes are matched with PRs at the time that the fix
> is made (rather than some time after the fact).
I appreciate the feedback!
> Marek Polacek via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > […]
> > My thinking is that for:
> >
> > * rejects-valid: use the existing dg-xfail-if
> > * accepts-valid: use the new dg-accepts-invalid
> > * ICEs: use the new dg-ice
> >
> > dg-ice can be used like this:
> >
> > // { dg-ice "build_over_call" { target c++11 } }
> >
> > and it means that if the test still ICEs, you'll get a quiet XFAIL. If the
> > ICE is fixed, you'll get an XPASS; if the ICE is gone but there are errors,
> > you'll get an XPASS + FAIL. Then you can close the old PR.
>
> This is long overdue IMO, thanks for adding it.
>
> > Similarly, dg-accepts-invalid:
> >
> > // { dg-accepts-invalid "PR86500" }
> >
> > means that if the test still compiles without errors, you'll get a quiet
> > XFAIL.
> > If we start giving errors, you'll get an XPASS.
> >
> > If the bug is fixed, simply remove the directive.
> >
> > The patch implementing these new directives is appended. Once/if this is
> > accepted, I can start adding the old tests we have in our Bugzilla. (I'm
> > only concerned about the c++ component, if that wasn't already clear.)
> >
> > The question is what makes the bug "old": is it one year without it being
> > assigned? 6 months? 3 months? Note: I *don't* propose to add every test
> > for
> > every new PR, just the reasonably old ones that are useful/important. Such
> > additions should be done in batches, so that we don't have dozens of
> > commits,
> > each of them merely adding a single test.
>
> IMO it should be OK to add a testcase for any open PR, if someone think
> it's useful, regardless of age and without being forced to batch the
> commits. I.e. I think it should come under the “obvious” rule and
> people should just use their judgement about when it's appropriate.
> Adding XFAILing tests shouldn't disturb anyone else very much.
Sounds good. I do think it should be left up to developers, and that such
tests can go in under the obvious rule -- you can hardly break stuff.
My point about the batches was that if you know you're going to add 10 tests,
it's better to add them in 1 squashed commit rather than 10 separate commits.
> I guess there's a possibility that some tests happen to pass already
> on some targets. That's more likely with middle-end and backend bugs
> rather than frontend stuff though. Perhaps for those it would make
> sense to have a convention in which the failing testcase is restricted
> (at the whole-test level) to the targets that the person committing the
> testcase has actually tried. Maybe with a comment on the dg-ice etc.
> to remind people to reconsider the main target selector when un-XFAILing
> the test.
Interesting point. With my frontend hat on, I hadn't really thought of
this much, but the dg-ice directive allows you to specify the targets and
specific options when to expect an ICE. So you could run a test everywhere
but only expect an ICE on aarch64.
Thanks,
Marek
