Re: Add extension options to control TAP and isolation tests

On Thu, Nov 22, 2018 at 10:01:26PM +0300, Nikolay Shaplov wrote:
> В письме от 21 ноября 2018 09:39:53 пользователь Michael Paquier написал:
>> Those are mentioned here as part of the additional test suites:
>> https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5
>
> Oh thanks (I feel urge to start editing documentation right now. I will
> surpress it, and do it, I hope, later.)

If you have a patch to improve the existing docs, please feel free to
submit those on a different thread. If you have suggestions about
improving this patch's docs, of course please feel free to suggest them
here!

> May I also ask a question, I came across. It is not part of the patch, but
> nevertheless...
> If I look in the code, regression test are sql files with expected output that
> are in src/test/regress. If I look in the documentation, all the tests are
> regression tests including TAP tests.
> https://www.postgresql.org/docs/11/regress.html
>
> what is the right way to look at it?

That's for the main regression test suite within src/test/regress, TAP
is also a regression test suite, the page of the link reflects the
general set of test suites available.

> So, I continued exploring your patch. First I carefully read changes in
> pgxs.mk. The only part of it I did not understand is
>
> .PHONY: submake
> -submake:
> ifndef PGXS
> +submake:
> +ifdef REGRESS
> $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X)
> endif
> +ifdef ISOLATION
> + $(MAKE) -C $(top_builddir)/src/test/isolation all
> +endif
> +endif # PGXS

This is to make sure that the necessary tools are compiled before
running the related tests. "submake:" needs to be moved out actually.
Thinking about it a bit more, we should also remove the "ifdef REGRESS"
and "ifdef ISOLATION" because there are some dependencies here. For
example TAP tests call pg_regress to initialize connection policy. TAP
tests may also use isolation_tester or such.

> I suppose it is because the purpose of PGXS is never explained, not in the
> documentation, not in the code comments, and PGXS := $(shell $(PG_CONFIG) --
> pgxs) sounds like some strange magic spell, that explains nothing. In what
> cases it is defined? In what cases it is not defined? What exactly does it
> store? Can we make things more easy to understand here?

That's part of a larger scope which is here:
https://www.postgresql.org/docs/11/extend-pgxs.html

> 2. As far as you said that TAP tests for bloom were never executed,
> I guess I should just ignore
>
> -
> -wal-check: temp-install
> - (prove_check)
>
> as a part of wrong way to execute TAP tests. (I fond no traces of "wal-check"
> string in the code, so I guess this build target is an invention of bloom
> authors)

Yes. It seems that it was useful for debugging at this time, but this
will rot if let as-is... I am pretty sure that most people don't use
that wrapper.

> 3. The rest are trivial, except changes for contrib/test_decoding/ and
> src/test/modules/brin I will explore them in my next postgres-dev time slot
> and then my review work will be done :-)

Thanks for the input, Nikolay!
--
Michael