Re: [HACKERS] Add regression tests for SET xxx
On Sun, Jul 7, 2013 at 1:15 PM, Robins Tharakan thara...@gmail.com wrote: Updated the patch: - Updated ROLE names as per Robert's feedback (regress_xxx) - Added test to serial_schedule Please see my comments on the LOCK tests related to these points and update accordingly. +BEGIN TRANSACTION; +INVALID_COMMAND; +ERROR: syntax error at or near INVALID_COMMAND +LINE 1: INVALID_COMMAND; +^ +SET SESSION AUTHORIZATION regress_role_var1; +ERROR: current transaction is aborted, commands ignored until end of transaction block We have a test for this error message in the transactions test, so I don't think we need another one here. This error is thrown at a very high level in e.g. exec_simple_query. The only dependence on the particular command is in IsTransactionExitStmt() - IOW, there's nothing special about SET, and if we're going to test this for SET ROLE and SET SESSION AUTHORIZATION, then why not for every single command we have? +SET DATESTYLE = ISO, SQL; +ERROR: invalid value for parameter DateStyle: iso, sql +DETAIL: Conflicting datestyle specifications. +SET DATESTYLE = SQL, ISO; +ERROR: invalid value for parameter DateStyle: sql, iso +DETAIL: Conflicting datestyle specifications. +SET DATESTYLE = ISO, POSTGRES; +ERROR: invalid value for parameter DateStyle: iso, postgres +DETAIL: Conflicting datestyle specifications. +SET DATESTYLE = POSTGRES, ISO; ...et cetera, ad nauseum. While there's something to be said for completeness, the chances that future maintainers of this code will update these regression tests with all the new permutations every time new options are added seem low. I recommend testing a representative sample of three of these, or so, and skipping the rest. +SET TIME ZONE INTERVAL '1 month +05:30' HOUR TO MINUTE; +ERROR: invalid value for parameter TimeZone: INTERVAL '@ 1 mon 5 hours 30 mins' Wow, what a terrible error message. This is not your patch's fault, but maybe we should look at improving that? I am inclined to think that these tests should be split up and moved into the files where similar things are being tested already. The SET TIME ZONE tests probably belong in horology.sql, the SET DATESTYLE tests with the related tests in interval.sql, and much of the other stuff in transactions.sql. -- 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] Add regression tests for SET xxx
On 18 June 2013 05:01, Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Updated the patch: - Updated ROLE names as per Robert's feedback (regress_xxx) - Added test to serial_schedule -- Robins Tharakan regress_variable_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] Add regression tests for SET xxx
On 18 June 2013 02:33, Robins Tharakan thara...@gmail.com wrote: Thanks ! PFA the updated patch. Also remove a trailing whitespace at the end of SQL script. -- Robins Tharakan On 17 June 2013 17:29, Szymon Guz mabew...@gmail.com wrote: On 26 May 2013 19:56, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) ( src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, the patch applies cleanly on code from trunk, however there are failing tests, diff attached. regards Szymon Hi, I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. thanks, Szymon Guz
Re: [HACKERS] Add regression tests for SET xxx
On Tue, Jun 18, 2013 at 7:01 PM, Szymon Guz mabew...@gmail.com wrote: Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. You should be able to do it yourself by creating a community account in postgresql.org. -- Michael -- 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] Add regression tests for SET xxx
On 18 June 2013 13:10, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Jun 18, 2013 at 7:01 PM, Szymon Guz mabew...@gmail.com wrote: Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. You should be able to do it yourself by creating a community account in postgresql.org. -- Michael Yea, I know. Unfortunately there is a bug and currently you cannot login to commitfest using a new account, or old, if you changed password like I did. I've got a bug confirmation from Magnus on pgsql-www list. thanks, Szymon
Re: [HACKERS] Add regression tests for SET xxx
On 18 June 2013 17:29, Kevin Grittner kgri...@ymail.com wrote: Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. It sounded like you felt this was Ready for Committer, so I set it that way. Let me know if you don't think it's to that point yet. Hi Kevin, yes, that's what I was thinking about. Thanks, Szymon
Re: [HACKERS] Add regression tests for SET xxx
Szymon Guz mabew...@gmail.com wrote: I've checked the patch. Applies cleanly. Tests pass this time :) Could you add me as a reviewer to commitfest website, set this patch a reviewed and add this email to the patch history? I cannot login to the commitfest app, there is some bug with that. It sounded like you felt this was Ready for Committer, so I set it that way. Let me know if you don't think it's to that point yet. -- Kevin Grittner 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] Add regression tests for SET xxx
On 26 May 2013 19:56, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) (src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, the patch applies cleanly on code from trunk, however there are failing tests, diff attached. regards Szymon regression.diffs 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] Add regression tests for SET xxx
Thanks ! PFA the updated patch. Also remove a trailing whitespace at the end of SQL script. -- Robins Tharakan On 17 June 2013 17:29, Szymon Guz mabew...@gmail.com wrote: On 26 May 2013 19:56, Robins Tharakan thara...@gmail.com wrote: Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) (src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Hi, the patch applies cleanly on code from trunk, however there are failing tests, diff attached. regards Szymon regress_variable_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
[HACKERS] Add regression tests for SET xxx
Hi, Please find attached a patch to take code-coverage of SET (SESSION / SEED / TRANSACTION / DATESTYLE / TIME ZONE) (src/backend/commands/variable.c) from 65% to 82%. Any and all feedback is welcome. -- Robins Tharakan regress_variable.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
