Re: Fix bloom WAL tap test

On Wed, Nov 8, 2017 at 1:58 AM, Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> On Tue, Nov 7, 2017 at 4:34 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.global
include $(top_srcdir)/contrib/contrib-global.mk
endif

+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