Re: [HACKERS] Patch to add regression tests for SCHEMA
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
On 9 July 2013 08:57, Fabien COELHO coe...@cri.ensmp.fr wrote: I cannot apply the patch, it seems to be truncated: Hi Fabien, Please find attached the updated patch. It seems the only difference between v5 and v6 is probably a newline at the end (Line 291 was the last line), in fact meld doesn't even show anything. I'll try to be more careful though. git reset --hard HEAD git pull patch -p1 ../regress_schema_v6.patch patch -p1 -R ../regress_schema_v6.patch patch -p1 ../regress_schema_v6.patch make clean ./configure --enable-depend --enable-coverage --enable-cassert --enable-debug make -j3 check Do let me know if something is still amiss. -- Robins Tharakan
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi Fabien, Please find attached the updated patch. It seems the only difference between v5 and v6 is probably a newline at the end (Line 291 was the last line), in fact meld doesn't even show anything. I'll try to be more careful though. git reset --hard HEAD git pull patch -p1 ../regress_schema_v6.patch patch -p1 -R ../regress_schema_v6.patch patch -p1 ../regress_schema_v6.patch make clean ./configure --enable-depend --enable-coverage --enable-cassert --enable-debug make -j3 check Do let me know if something is still amiss. -- Robins Tharakan regress_schema_v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
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 at line 290: I have this: sh cksum ../regress_schema_v5.patch 985580529 11319 ../regress_schema_v5.patch -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi Fabien, Appreciate you being able to check right away. Seems something went wrong with these set of patches... Would check again and resubmit them soon. -- Robins Tharakan On 9 July 2013 10:57, Fabien COELHO coe...@cri.ensmp.fr wrote: 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 at line 290: I have this: sh cksum ../regress_schema_v5.patch 985580529 11319 ../regress_schema_v5.patch -- Fabien.
Re: [HACKERS] Patch to add regression tests for SCHEMA
On 3 July 2013 10:19, Robert Haas robertmh...@gmail.com wrote: 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. Hi Robert, 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 -- Robins Tharakan regress_schema_v5.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
On Wed, May 8, 2013 at 9:19 PM, Robins Tharakan thara...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
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_schsome 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 in the future a new schema name is added, the test will fail. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi, 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). -- Robins Tharakan On 8 May 2013 13:01, Fabien COELHO coe...@cri.ensmp.fr wrote: 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_schsome 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 in the future a new schema name is added, the test will fail. -- Fabien. regress_schema_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi, Here is an updated patch that uses different schema / role names for different tests (as per commitfest site feedback). -- Robins Tharakan On 18 March 2013 17:16, robins thara...@gmail.com wrote: Hi, Attached is an updated patch that uses better schema / role names. -- Robins Tharakan On 18 March 2013 12:59, robins thara...@gmail.com wrote: Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_schema_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
On Tue, May 7, 2013 at 7:26 PM, Robins Tharakan thara...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
Completely agree. Although the poster was kind enough to intimate me by email about his update there, but was wondering that if he hadn't, I wouldnt' have dreamt that there is a feedback on the site, two months down the line. -- Robins Tharakan On 8 May 2013 09:13, Robert Haas robertmh...@gmail.com wrote: On Tue, May 7, 2013 at 7:26 PM, Robins Tharakan thara...@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
Re: [HACKERS] Patch to add regression tests for SCHEMA
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch to add regression tests for SCHEMA
Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi, Attached is an updated patch that uses better schema / role names. -- Robins Tharakan On 18 March 2013 12:59, robins thara...@gmail.com wrote: Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services regress_schema_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Patch to add regression tests for SCHEMA
Hi, Please find attached a patch to take 'make check' code-coverage of SCHEMA from 33% to 98%. Any feedback is more than welcome. 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. -- Robins Tharakan regress-schema.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
