pg_regress: Treat child process failure as test failure

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