Thread: Add regression tests for ROLE (USER)

This updated version works for me and addresses previous comments.

I think that such tests are definitely valuable, especially as many corner 
cases which must trigger errors are covered.

I recommend to apply it.

> Please find an updated patch as per comments on Commitfest (comments
> replicated below for ease of understanding).
>
> Feedback 1:
> fc: role_ro2/3 used twice?
> rt: Corrected in this update.
>
> Feedback 2:
> fc: I do not understand why "asdf" conveys anything about an expected
> failure. Association of Scientists, Developers and Faculties? :-)
> rt: ASDF is a pattern that I learnt in one of the tests (SEQUENCE?) that
> pre-existed when I started working. Its a slang for arbit text that I just
> reused thinking that it is normal practice here. Anyway, have corrected
> that in this update.
>
> Feedback 3:
> fc: 2030/1/1 -> 2030-01-01? maybe use a larger date?
> rt: 2030/1/1 date is not a failure point of the test. It needs to be a
> valid date (but sufficiently distant that so that tests don't fail). I
> tried setting this to 2200/1/1 and I get the same error message. Let me
> know if this still needs to be a large date.
> fb: VALID UNTIL '9999-12-31' works for me...
> rt: I thought 20 years is a date sufficiently far ahead to ensure that this
> test doesn't fail. Sure, have updated the test to use 9999/1/1. Also, have
> added more tests at the end to ensure date-checks are also being validated
> in ALTER ROLE VALID UNTIL.

-- 
Fabien.

On Thu, May 9, 2013 at 4:29 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> This updated version works for me and addresses previous comments.
>
> I think that such tests are definitely valuable, especially as many corner
> cases which must trigger errors are covered.
>
> I recommend to apply it.

I'm attaching an update of this patch that renames both the new test
file (user->role), and the regression users that get created.  It also
fixes the serial schedule to match the parallel schedule.

However, before it can get committed, I think this set of tests needs
streamlining.  It does seem to me valuable, but I think it's wasteful
in terms of runtime to create so many roles, do just one thing with
them, and then drop them.  I recommend consolidating some of the
tests.  For example:

+-- Should work. ALTER ROLE with (UN)ENCRYPTED PASSWORD
+CREATE ROLE regress_rol_rol14;
+ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD 'abc';
+DROP ROLE regress_rol_rol14;
+CREATE ROLE regress_rol_rol15;
+ALTER ROLE regress_rol_rol15 WITH UNENCRYPTED PASSWORD 'abc';
+DROP ROLE regress_rol_rol15;
+
+-- Should fail. ALTER ROLE with (UN)ENCRYPTED PASSWORD but no password value
+CREATE ROLE regress_rol_rol16;
+ALTER ROLE regress_rol_rol16 WITH ENCRYPTED PASSWORD;
+DROP ROLE regress_rol_rol16;
+CREATE ROLE regress_rol_rol17;
+ALTER ROLE regress_rol_rol17 WITH UNENCRYPTED PASSWORD;
+DROP ROLE regress_rol_rol17;
+
+-- Should fail. ALTER ROLE with both UNENCRYPTED and ENCRYPTED
+CREATE ROLE regress_rol_rol18;
+ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc';
+DROP ROLE regress_rol_rol18;

There's no reason that couldn't be written this way:

CREATE ROLE regress_rol_rol14;
ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD 'abc';
ALTER ROLE regress_rol_rol14 WITH UNENCRYPTED PASSWORD 'abc';
ALTER ROLE regress_rol_rol14 WITH ENCRYPTED PASSWORD;
ALTER ROLE regress_rol_rol14 WITH UNENCRYPTED PASSWORD;
ALTER ROLE regress_rol_rol14 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc';
DROP ROLE regress_rol_rol14;

Considering the concerns already expressed about the runtime of the
test, I think it's important to minimize the number of create/drop
role cycles that the tests perform.

Generally, I think that the tests which return a syntax error are of
limited value and should probably be dropped.  That is unlikely to get
broken by accident.  If the syntax error is being thrown by something
outside of bison proper, that's probably worth testing.  But I think
that testing random syntax variations is a pretty low-value
proposition.

Setting this to "Waiting on Author".

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

> Generally, I think that the tests which return a syntax error are of 
> limited value and should probably be dropped.

I think that it is not that simple: it is a good value to check that the 
syntax error message conveys a useful information for the user, and that 
changes to the parser rules do not alter good quality error messages.

Moreover, the cost of such tests in time must be quite minimal.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> Generally, I think that the tests which return a syntax error are of 
>> limited value and should probably be dropped.

> I think that it is not that simple: it is a good value to check that the 
> syntax error message conveys a useful information for the user, and that 
> changes to the parser rules do not alter good quality error messages.

It's good to check those things when a feature is implemented.  However,
once it's done, the odds of the bison parser breaking are very low.
Thus, the benefit of testing that over again thousands of times a day
is pretty tiny.

> Moreover, the cost of such tests in time must be quite minimal.

I'm not convinced (see above) and in any case the benefit is even more
minimal.

(Note that semantic errors, as opposed to syntax errors, are a different
question.)
        regards, tom lane

On 2013-07-07 11:11:49 -0400, Tom Lane wrote:
> Fabien COELHO <coelho@cri.ensmp.fr> writes:
> >> Generally, I think that the tests which return a syntax error are of 
> >> limited value and should probably be dropped.
> 
> > I think that it is not that simple: it is a good value to check that the 
> > syntax error message conveys a useful information for the user, and that 
> > changes to the parser rules do not alter good quality error messages.
> 
> It's good to check those things when a feature is implemented.  However,
> once it's done, the odds of the bison parser breaking are very low.
> Thus, the benefit of testing that over again thousands of times a day
> is pretty tiny.

There has been quite some talk about simplifying the grammar/scanner
though, if somebody starts to work on that *good* tests on syntax errors
might actually be rather worthwhile. Imo there's the danger of reducing
the specifity of error messages when doing so.
Granted, that probably mostly holds true for things actually dealing
with expressions...

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

>> I think that it is not that simple: it is a good value to check that the
>> syntax error message conveys a useful information for the user, and that
>> changes to the parser rules do not alter good quality error messages.
>
> It's good to check those things when a feature is implemented.  However,
> once it's done, the odds of the bison parser breaking are very low.

I do not know that. When the next version of bison is out (I have 2.5 from 
2011 on my laptop, 2.7.1 was released on 2013-04-15), or if a new "super 
great acme incredible" drop-in replacement is proposed, you would like to 
see the impact, whether positive or negative, it has on error messages 
before switching.

> Thus, the benefit of testing that over again thousands of times a day
> is pretty tiny.

Sure, I agree that thousands of times per day is an overkill for syntax 
errors. But once in a while would be good, and for that you need to have 
them somewhere, and the current status is "nowhere".

-- 
Fabien.

On Mon, Jul 15, 2013 at 10:23 AM, Robins Tharakan <tharakan@gmail.com> wrote:
> It still doesn't address the excessive (syntactical) checks tough. I am
> still unclear as to how to identify which checks to skip. (As in, although I
> have a personal preference of checking everything, my question probably
> wasn't clear in my previous email. I was just asking 'how' to know which
> checks to not perform.) Should a 'syntax error' in the message be considered
> as an unnecessary check? If so, its easier for me to identify which checks
> to skip, while creating future tests.

Yes, I think you can just leave out the syntax errors.

I simply don't understand how we can be getting any meaningful test
coverage out of those cases.  I mean, if we want to check every bit of
syntax that could lead to a syntax error, we could probably come up
with a near-infinite number of test cases:

CREAT TABLE foo (x int);
CREATE TABL foo (x int);
CREATER TABLE foo (x int);
CREATE TABLES foo (x int);
CREATE CREATE TABLE foo (x int);
CREATE TABLE foo [x int);
CREATE TABLE foo (x int];
CREATE TABLE foo [x int];
CREATE TABLE (x int);
CREATE foo (x int);

...and on and on it goes.  Once we start speculating that bison
doesn't actually produce a grammar that matches the productions
written in gram.y, there's no limit to what can go wrong, and no
amount of test coverage can be too large.  In practice, the chances of
catching anything that way seem minute.  If bison breaks in such a way
that all currently accepted grammatical constructs are still accepted
and work, but something that shouldn't have been accepted is, we'll
just have to find that out in some way other than our regression
tests.  I think it's very unlikely that such a thing will happen, and
even if it does, I don't really see any reason to suppose that the
particular tests you've included here will be the ones that catch the
problem.

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

> I simply don't understand how we can be getting any meaningful test
> coverage out of those cases.  I mean, if we want to check every bit of
> syntax that could lead to a syntax error, we could probably come up
> with a near-infinite number of test cases:

I think that it would be enough to check for expected 
keywords/identifier/stuff whether the syntax error reported make sense. 
Basically the parser reports the first found inconsistency.
 1. CREAT TABLE foo (x int); 2. CREATE TABL foo (x int); 3. CREATER TABLE foo (x int); -- same as 1 4. CREATE TABLES
foo(x int); -- same as 2 5. CREATE CREATE TABLE foo (x int); -- hmmm. 6. CREATE TABLE foo [x int); 7. CREATE TABLE foo
(xint]; 8. CREATE TABLE foo [x int]; -- same as 6 & 7 9. CREATE TABLE (x int); A. CREATE foo (x int); -- same as 2
 

This level of testing can be more or less linear in the number of token.

-- 
Fabien.

On Mon, Jul 15, 2013 at 11:48 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> I simply don't understand how we can be getting any meaningful test
>> coverage out of those cases.  I mean, if we want to check every bit of
>> syntax that could lead to a syntax error, we could probably come up
>> with a near-infinite number of test cases:
>
> I think that it would be enough to check for expected
> keywords/identifier/stuff whether the syntax error reported make sense.
> Basically the parser reports the first found inconsistency.
>
>  1. CREAT TABLE foo (x int);
>  2. CREATE TABL foo (x int);
>  3. CREATER TABLE foo (x int); -- same as 1
>  4. CREATE TABLES foo (x int); -- same as 2
>  5. CREATE CREATE TABLE foo (x int); -- hmmm.
>  6. CREATE TABLE foo [x int);
>  7. CREATE TABLE foo (x int];
>  8. CREATE TABLE foo [x int]; -- same as 6 & 7
>  9. CREATE TABLE (x int);
>  A. CREATE foo (x int); -- same as 2
>
> This level of testing can be more or less linear in the number of token.

Maybe so, but that's still a huge number of regression tests that in
practice won't find anything.  But they will take work to maintain.

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