Re: pg_regress: Treat child process failure as test failure
> On 22 Feb 2023, at 21:55, Tom Lane wrote: > > Daniel Gustafsson writes: >>> On 22 Feb 2023, at 21:33, Andres Freund 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
Re: pg_regress: Treat child process failure as test failure
Daniel Gustafsson writes: >> On 22 Feb 2023, at 21:33, Andres Freund 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
Re: pg_regress: Treat child process failure as test failure
> On 22 Feb 2023, at 21:33, Andres Freund 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
Re: pg_regress: Treat child process failure as test failure
Hi, On 2023-02-22 15:10:11 +0100, Daniel Gustafsson wrote: > > On 26 Nov 2022, at 22:46, Daniel Gustafsson 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
Re: pg_regress: Treat child process failure as test failure
> On 26 Nov 2022, at 22:46, Daniel Gustafsson 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 v3-0001-pg_regress-Consider-a-failed-test-process-as-a-fa.patch Description: Binary data
Re: pg_regress: Treat child process failure as test failure
> On 26 Nov 2022, at 21:55, Andres Freund 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/ v2-0001-Consider-a-failed-test-process-as-a-failed-test.patch Description: Binary data
Re: 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)
> {
> boolignore = 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
