Thread: Patch to add regression tests for SCHEMA

robins escribió:
> Hi,
>
> Please find attached a patch to take 'make check' code-coverage of SCHEMA
> from 33% to 98%.
>
> Any feedback is more than welcome.

I think you should use more explicit names for shared objects such as
roles -- i.e. not "r1" but "regression_user_1" and so on. (But be
careful about role names used by other tests).  The issue is that these
tests might be run in a database that contains other stuff; certainly we
don't want to drop or otherwise affect previously existing roles.

> p.s.: I am currently working on more regression tests (USER / VIEW /
> DISCARD etc). Please let me know if I need to post these as one large
> patch, instead of submitting one patch at a time.

I think separate patches is better.  Are you adding these patches to the
upcoming commitfest, for evaluation?  https://commitfest.postgresql.org

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, May 7, 2013 at 7:26 PM, Robins Tharakan <tharakan@gmail.com> wrote:
> Here is an updated patch that uses different schema / role names for
> different tests (as per commitfest site feedback).

I'm not sure what's going on here.  Reviews are to be posted to
pgsql-hackers, and then linked from the CommitFest site.  Putting
reviews only on the CommitFest site is bad practice.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Reviews are to be posted to pgsql-hackers, and then linked from the 
> CommitFest site.  Putting reviews only on the CommitFest site is bad 
> practice.

Indeed. Sorry, shame on me!

I had not the original mail in my mailbox because I deleted it, I did not 
want to create a new thread because this is /also/ bad practice as I was 
recently told, and I was not motivated by fetching and reinstating the 
messages in my mailbox for a short one-liner review.

Weel, I'll do better next time.

Anyway, all Robins' test cases are basically a very good thing, especially 
as he tries corner cases, including checking for expected errors and 
permission denials.

-- 
Fabien.

Dear Robins,

> Here is an updated patch that uses different schema / role names for
> different tests (as per commitfest site feedback).

Short review about this version of the patch:

This patch work for me.

This test is a good thing and allows schema to be thoroughly tested, 
including corner cases which must fail because of errors or permissions.

Two remarks:
 - test 2 bis: why name 'pg_asdf'? why not 'pg_schema_sch<some number>'   to be homogeneous with other tests?
 - test 3: why not WHERE schema_name='schema_sch3' instead of two   negative comparisons? ISTM that if for some reason
inthe future a new   schema name is added, the test will fail.
 

-- 
Fabien.

> Please find attached an updated patch with the said changes. I'll try to 
> update the other patches (if they pertain to this feedback) and update 
> on their respective threads (as well as on Commitfest).

Ok, this new version addresses my questions.

The patch works for me (nothing to compile, the added tests pass).

I recommend its inclusion as it tests corner cases especially about 
permissions and error conditions, some of which may have security 
implications if they were to fail some day. So this is a good thing.

The above remark applies to all systematic but not redundant regression 
tests submitted. If the overall test was to be deemed too long and slow 
for developers, I would suggest to have a two-tier system with basic and 
fast tests for devs and longer tests for the build farm.

-- 
Fabien.

On Wed, May 8, 2013 at 9:19 PM, Robins Tharakan <tharakan@gmail.com> wrote:
> Please find attached an updated patch with the said changes.
> I'll try to update the other patches (if they pertain to this feedback) and
> update on their respective threads (as well as on Commitfest).

This patch updates the parallel schedule but not the serial schedule.
Please try to remember to update both files when adding new test
files.  Also, please observe the admonitions in the parallel schedule,
at top of file:

# By convention, we put no more than twenty tests in any one parallel group;
# this limits the number of connections needed to run the tests.

In this particular case, I think that adding a new set of tests isn't
the right thing anyway.  Schemas are also known as namespaces, and the
existing "namespace" test is where related test cases live.  Add these
tests there instead.

Rename regression users to regress_rol_nsp1 etc. per convention
established in the CREATE OPERATOR patch.

Setting patch to "Waiting on Author".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> PFA an updated patch:
> - Renamed ROLEs as per new feedback (although the previous naming was also
> as per an earlier feedback)
> - Merged tests into namespace

I do not see any difference with v4. I guess you sent the earlier version.

-- 
Fabien.

>> PFA an updated patch:
>> - Renamed ROLEs as per new feedback (although the previous naming was also
>> as per an earlier feedback)
>> - Merged tests into namespace
>
> I do not see any difference with v4. I guess you sent the earlier version.

Sorry, wrong thread, this apply to SEQUENCE tests.

-- 
Fabien.

Hello Robins,

> PFA an updated patch:
> - Renamed ROLEs as per new feedback (although the previous naming was also
> as per an earlier feedback)
> - Merged tests into namespace

I cannot apply the patch, it seems to be truncated:
 sh> git apply ../regress_schema_v5.patch ### warnings about trailing whitespace, then: fatal: corrupt patch at line
291

Another go with "patch":
 sh> patch -p1 < ../regress_schema_v5.patch ... patch unexpectedly ends in middle of line patch: **** malformed patch
atline 290:
 

I have this:
 sh> cksum ../regress_schema_v5.patch 985580529 11319 ../regress_schema_v5.patch

-- 
Fabien.

I've looked this version.

The only reservation I have is that when changing the owner of a schema, 
the new owner is not always checked. I would suggest to query the new 
owner to check that it matches (5, 11, 12), just as you do in 3.

Also, reowning is tested several times (5, 11, 12). I would suggest to 
remove 12 which does not bring much new things after both 5 and 11 get 
passed ?

Otherwise the patch applies (with a minor warning about spaces on line 33) 
passes for me, and brings valuable new test coverage.

-- 
Fabien.