Re: Add extension options to control TAP and isolation tests

В письме от 21 ноября 2018 09:39:53 пользователь Michael Paquier написал:

> > For me name "output_iso" means nothing. iso is something about CD/DVD or
> > about standards. I would not guess that iso stands for isolation if I did
> > not know it already. isolation_output is more sensible: I have heard that
> > there are some isolation tests, this must be something about it. May be
> > it would be better to change it to isolation_output everywhere instead of
> > changing to output_iso
> That's just a default configuration used by the isolation tester.
> That's not much bothering with in my opinion for this patch, as the
> patch is here to make sure that the default configuration gets used
> where it had better be used. Changing this default value would be of
> course doable technically, but that's around for years to changing it
> does not seem like good idea.

Ok. If it is long time tradition, let it be :-)

> > I tried to find definition in documentation what does "isolation test"
> > exactly means, but did not find. There is some general words about TAP
> > tests in main postgres documentation
> > https://www.postgresql.org/docs/11/regress-tap.html,
> > but I would not understand anything from it if I did not already know how
> > it works.
>
> 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.)
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?

> > In current extend-pgxs documentation there is some explanation about
> > regression test, it sensible enough. Since TAP and isolation tests are
> > introduced now, there should be same short explanation for both of
> > them.
>
> I see your point here, and it is true that documentation ought to be
> better. So I have written out a new set of paragraphs which explain the
> whereabouts of the new variables a bit more in depth in the section of
> extend-pgxs.

Oh thanks! Now it is much more clear.

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

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?

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)

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 :-)

--
Do code for fun.