Обсуждение: pg_regress: Treat child process failure as test failure

In the thread about TAP format out in pg_regress, Andres pointed out [0] that
we allow a test to pass even if the test child process failed.  While its
probably pretty rare to have a test pass if the process failed, this brings a
risk for false positives (and it seems questionable that any regress test will
have a child process failing as part of its intended run).

The attached makes child failures an error condition for the test as a belts
and suspenders type check. Thoughts?

--
Daniel Gustafsson        https://vmware.com/

[0] https://postgr.es/m/20221122235636.4frx7hjterq6bmls@awork3.anarazel.de

Hi,

On 2022-11-26 21:11:39 +0100, Daniel Gustafsson wrote:
> In the thread about TAP format out in pg_regress, Andres pointed out [0] that
> we allow a test to pass even if the test child process failed.  While its
> probably pretty rare to have a test pass if the process failed, this brings a
> risk for false positives (and it seems questionable that any regress test will
> have a child process failing as part of its intended run).

> The attached makes child failures an error condition for the test as a belts
> and suspenders type check. Thoughts?

I wonder if it's the right thing to treat a failed psql that's then also
ignored as "failed (ignored)". Perhaps it'd be better to move the statuses[i]
!= 0 check to before the if (differ)?


> -            if (differ)
> +            if (differ || statuses[i] != 0)
>              {
>                  bool        ignore = false;
>                  _stringlist *sl;
> @@ -1815,7 +1815,7 @@ run_single_test(const char *test, test_start_function startfunc,
>          differ |= newdiff;
>      }
>  
> -    if (differ)
> +    if (differ || exit_status != 0)
>      {
>          status(_("FAILED"));
>          fail_count++;

It certainly is a bit confusing that we print a psql failure separately from
the if "FAILED" vs "ok" bit.

Greetings,

Andres Freund

> On 26 Nov 2022, at 21:55, Andres Freund <andres@anarazel.de> wrote:
> On 2022-11-26 21:11:39 +0100, Daniel Gustafsson wrote:

>> The attached makes child failures an error condition for the test as a belts
>> and suspenders type check. Thoughts?
>
> I wonder if it's the right thing to treat a failed psql that's then also
> ignored as "failed (ignored)". Perhaps it'd be better to move the statuses[i]
> != 0 check to before the if (differ)?

I was thinking about that too, but I think you're right.  The "ignore" part is
about the test content and not the test run structure.

> It certainly is a bit confusing that we print a psql failure separately from
> the if "FAILED" vs "ok" bit.

I've moved the statuses[i] check before the differ check, such that there is a
separate block for this not mixed up with the differs check and printing.  It
does duplicate things a little bit but also makes it a lot clearer.

--
Daniel Gustafsson        https://vmware.com/

> On 26 Nov 2022, at 22:46, Daniel Gustafsson <daniel@yesql.se> wrote:

> I've moved the statuses[i] check before the differ check, such that there is a
> separate block for this not mixed up with the differs check and printing.

Rebased patch to handle breakage of v2 due to bd8d453e9b.

--
Daniel Gustafsson

Hi,

On 2023-02-22 15:10:11 +0100, Daniel Gustafsson wrote:
> > On 26 Nov 2022, at 22:46, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> > I've moved the statuses[i] check before the differ check, such that there is a
> > separate block for this not mixed up with the differs check and printing.
> 
> Rebased patch to handle breakage of v2 due to bd8d453e9b.

I think we probably should just apply this? The current behaviour doesn't seem
right, and I don't see a downside of the new behaviour?

Greetings,

Andres Freund

> On 22 Feb 2023, at 21:33, Andres Freund <andres@anarazel.de> wrote:
> On 2023-02-22 15:10:11 +0100, Daniel Gustafsson wrote:

>> Rebased patch to handle breakage of v2 due to bd8d453e9b.
>
> I think we probably should just apply this? The current behaviour doesn't seem
> right, and I don't see a downside of the new behaviour?

Agreed, I can't think of a regression test where we wouldn't want this.  My
only concern was if any of the ECPG tests were doing something odd that would
break from this but I can't see anything.

--
Daniel Gustafsson

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 22 Feb 2023, at 21:33, Andres Freund <andres@anarazel.de> wrote:
>> On 2023-02-22 15:10:11 +0100, Daniel Gustafsson wrote:
>>> Rebased patch to handle breakage of v2 due to bd8d453e9b.

>> I think we probably should just apply this? The current behaviour doesn't seem
>> right, and I don't see a downside of the new behaviour?

> Agreed, I can't think of a regression test where we wouldn't want this.  My
> only concern was if any of the ECPG tests were doing something odd that would
> break from this but I can't see anything.

+1.  I was a bit surprised to realize that we might not count such
a case as a failure.

            regards, tom lane

> On 22 Feb 2023, at 21:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 22 Feb 2023, at 21:33, Andres Freund <andres@anarazel.de> wrote:
>>> On 2023-02-22 15:10:11 +0100, Daniel Gustafsson wrote:
>>>> Rebased patch to handle breakage of v2 due to bd8d453e9b.
>
>>> I think we probably should just apply this? The current behaviour doesn't seem
>>> right, and I don't see a downside of the new behaviour?
>
>> Agreed, I can't think of a regression test where we wouldn't want this.  My
>> only concern was if any of the ECPG tests were doing something odd that would
>> break from this but I can't see anything.
>
> +1.  I was a bit surprised to realize that we might not count such
> a case as a failure.

Done that way, thanks!

--
Daniel Gustafsson