Thread: Add regression tests for SET xxx

On Tue, Jun 18, 2013 at 7:01 PM, Szymon Guz <mabewlun@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

Szymon Guz <mabewlun@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

On Sun, Jul 7, 2013 at 1:15 PM, Robins Tharakan <tharakan@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