Re: [PATCH] Tests for reloptions

В письме от 3 октября 2017 11:48:43 пользователь Michael Paquier написал:

> > src/backend/access/common/reloptions.c get only 7 lines, it was quite
> > covered by existing test, but all most of the access methods gets some
> > coverage increase:
> >
> > src/backend/access/brin 1268 -> 1280 (+18)
> > src/backend/access/gin 2924 -> 2924 (0)
> > src/backend/access/gist 1673 -> 2108 (+435)
> > src/backend/access/hash 1580 -> 1638 (+58)
> > src/backend/access/heap 2863 -> 2866 (+3)
> > src/backend/access/nbtree 2565 -> 2647 (+82)
> > src/backend/access/spgist 2066 -> 2068 (+2)
> >
> > Though I should say that incredible coverage boost for gist, is the result
> > of not steady results of test run. The real value should be much less...
> +-- Test buffering and fillfactor reloption takes valid values
> +create index gist_pointidx2 on gist_point_tbl using gist(p) with
> (buffering = on, fillfactor=50);
> +create index gist_pointidx3 on gist_point_tbl using gist(p) with
> (buffering = off);
> +create index gist_pointidx4 on gist_point_tbl using gist(p) with
> (buffering = auto);
> Those are the important bits doing the boost in gist visibly. To be kept.
>
> -CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random float8_ops);
> +CREATE INDEX hash_f8_index ON hash_f8_heap USING hash (random
> float8_ops) WITH (fillfactor=60);
> Woah. So that alone increases hash by 58 steps.
Might be... As I have noticed, tests for indexes filled with random data test
coverage, gives random coverage. There needed more random data to give steady
results. I am gathering statistics now, and later I hope I will offer another
patch(es) to fix it. So not all of 58 lines might be result of this patch :-)

> +CREATE INDEX bloomidx2 ON tst USING bloom (i, t) WITH (length=0);
> +ERROR: value 0 out of bounds for option "length"
> +DETAIL: Valid values are between "1" and "4096".
> contrib/bloom contributes to the coverage of reloptions.c thanks to
> its coverage of the add_ routines when the library is loaded. And
> those don't actually improve any coverage, right?
I actually run only simple "make check". running "make check" for bloom will
also add some add_ routines to coverage.

> > Nevertheless tests touches the reloptions related code, checks for proper
> > error handling, and it is good.
>
> Per your measurements reloptions.c is improved by 1.7%, or 7 lines as
> you say. Five of them are in parse_one_reloption for integer (2) and
> reals (2), plus one error at the top. Two are in transformRelOptions
> for a valid namespace handling. In your patch reloptions.sql is 141
> lines. Couldn't it be shorter with the same improvements? It looks
> that a lot of tests are overlapping with existing ones.
>
> > I think we should commit it.
>
> My take is that a lighter version could be committed instead. My
> suggestion is to keep the new test reloptions minimal so as it tests
> the relopt types and their bounds, and I think that you could remove
> the same bounding checks in the other tests that you added: bloom,
> gist, etc.

I've been thinking a lot, and rereading the patch. When I reread it I've been
thinking that I would like to add more tests to it now... ;-)

If the only purpose of tests is to get better coverage, then I would agree
with you. But I've been thinking that tests also should check that everything
behaves as expected (or as written in documentation).

I would say that options names is a of part of SQL dialect that postgres uses,
kind of part of the syntax. It is good to be sure that they still supported.
So I would add a test for every option heap supports.

Checking minimum and maximum values are more disputable. The argumentation for
it is that min and max are written in documentation, and it is good to check
that postgres behaves according to documentation...

But it you tell me for sure that test only for code coverage and should not do
anything more, than I surely remove all tests that are not increase test
coverage.

--
Do code for fun. Can do it for money (Perl & C/C++ ~10h/week)