Re: [HACKERS] Fix bloom WAL tap test

Поиск
Список
Период
Сортировка
От Michael Paquier
Тема Re: [HACKERS] Fix bloom WAL tap test
Дата
Msg-id CAB7nPqQiHK8s1iF3EFamHJg6230yQRdDBp70n-bUuc2g07KTTw@mail.gmail.com
обсуждение исходный текст
Ответ на [HACKERS] Fix bloom WAL tap test  (Alexander Korotkov <a.korotkov@postgrespro.ru>)
Ответы Re: [HACKERS] Fix bloom WAL tap test
Список 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 по дате отправления:

Предыдущее
От: Michael Paquier
Дата:
Сообщение: Re: [HACKERS] Fix bloom WAL tap test
Следующее
От: Michael Paquier
Дата:
Сообщение: Re: [HACKERS] [COMMITTERS] pgsql: Expand empty end tag