Re: [HACKERS] Fix bloom WAL tap test - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: [HACKERS] Fix bloom WAL tap test
Date
Msg-id CAB7nPqQiHK8s1iF3EFamHJg6230yQRdDBp70n-bUuc2g07KTTw@mail.gmail.com
Whole thread Raw
In response to [HACKERS] Fix bloom WAL tap test  (Alexander Korotkov <a.korotkov@postgrespro.ru>)
Responses Re: [HACKERS] Fix bloom WAL tap test
List pgsql-hackers
On Wed, Nov 8, 2017 at 1:58 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Tue, Nov 7, 2017 at 4:34 PM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
>> I understood the necessity of this patch and reviewed two patches.
>
> Good, thank you.

That's clearly a bug fix.

>> diff --git a/contrib/bloom/Makefile b/contrib/bloom/Makefile
>> index 13bd397..c1566d4 100644
>> --- a/contrib/bloom/Makefile
>> +++ b/contrib/bloom/Makefile
>> @@ -20,5 +20,7 @@ include $(top_builddir)/src/Makefile.global
>>  include $(top_srcdir)/contrib/contrib-global.mk
>>  endif
>>
>> +check: wal-check
>> +
>>  wal-check: temp-install
>>         $(prove_check)
>
>
> I've tried this version Makefile.  And I've seen the only difference: when
> tap tests are enabled, this version of Makefile runs tap tests before
> regression tests.  While my version of Makefile runs tap tests after
> regression tests.  That seems like more desirable behavior for me.  But it
> would be sill nice to simplify Makefile.

Why do you care about the order of actions? There is no dependency
between each test and it seems to me that it should remain so to keep
a maximum flexibility. This does not sacrifice coverage as well. In
short, Sawada-san's suggestion looks like a thing to me. One piece
that it is missing though is that installcheck triggers only the
SQL-based tests, and it should also trigger the TAP tests. So I think
that you should instead do something like that:

--- a/contrib/bloom/Makefile
+++ b/contrib/bloom/Makefile
@@ -20,5 +20,12 @@ include $(top_builddir)/src/Makefile.globalinclude $(top_srcdir)/contrib/contrib-global.mkendif

+installcheck: wal-installcheck
+
+check: wal-check
+
+wal-installcheck:
+   $(prove_installcheck)
+wal-check: temp-install   $(prove_check)

This works for me as I would expect it should. Could you update the
patch? That's way more simple than having to replicate again rules
like regresscheck and regressioncheck-install. I am switching the
patch back to "waiting on author" for now.

I can see that src/test/modules/commit_ts is missing the shot as well,
and I think that's the case as well of other test modules like
pg_dump's suite..
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: [HACKERS] Fix bloom WAL tap test
Next
From: Michael Paquier
Date:
Subject: Re: [HACKERS] [COMMITTERS] pgsql: Expand empty end tag