Horton

Thread

Commits

  1. Consider a failed process as a failed test in pg_regress

  1. pg_regress: Treat child process failure as test failure

    Daniel Gustafsson <daniel@yesql.se> — 2022-11-26T20:11:39Z

    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

  2. Re: pg_regress: Treat child process failure as test failure

    Andres Freund <andres@anarazel.de> — 2022-11-26T20:55:59Z

    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

  3. Re: pg_regress: Treat child process failure as test failure

    Daniel Gustafsson <daniel@yesql.se> — 2022-11-26T21:46:24Z

    > 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/

  4. Re: pg_regress: Treat child process failure as test failure

    Daniel Gustafsson <daniel@yesql.se> — 2023-02-22T14:10:11Z

    > 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

  5. Re: pg_regress: Treat child process failure as test failure

    Andres Freund <andres@anarazel.de> — 2023-02-22T20:33:45Z

    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

  6. Re: pg_regress: Treat child process failure as test failure

    Daniel Gustafsson <daniel@yesql.se> — 2023-02-22T20:42:21Z

    > 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

  7. Re: pg_regress: Treat child process failure as test failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-22T20:55:44Z

    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

  8. Re: pg_regress: Treat child process failure as test failure

    Daniel Gustafsson <daniel@yesql.se> — 2023-02-23T08:53:57Z

    > 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