Add regression tests for ROLE (USER)

Hi,

Please find attached a patch to take 'make check' code-coverage of ROLE
(USER) from 59% to 91%.

Any feedback is more than welcome.
--
Robins Tharakan

Hi,

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.

Let me know if you need anything else changed in this.
--
Robins Tharakan

On 20 March 2013 03:41, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

> Hi,
>
> Please find attached a patch to take 'make check' code-coverage of ROLE
> (USER) from 59% to 91%.
>
> Any feedback is more than welcome.
> --
> Robins Tharakan
>

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(at)cri(dot)ensmp(dot)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

> 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:
>
> 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.
>
>
Thanks Robert.

Although the idea of being repetitive was just about trying to make tests
simpler to infer for the next person, but I guess this example was
obviously an overkill. Point taken, would correct and revert with an
updated patch.

However, the other aspect that you mention, I am unsure if I understand
correctly. Do you wish that syntactical errors not be tested? If so,
probably you're referring to tests such as the one below, and then I think
it may get difficult at times to bifurcate how to chose which tests to
include and which to not. Can I assume that all errors that spit an error
messages with 'syntax error' are to be skipped, probably that'd be an easy
test for me to know what you'd consider important?

+ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASSWORD 'abc';
+ERROR: syntax error at or near "UNENCRYPTED"
+LINE 1: ALTER ROLE regress_rol_rol18 WITH ENCRYPTED UNENCRYPTED PASS...

Personally, I think all tests are important. Unless there is a clear
understanding that aiming for 100% code-coverage isn't the goal, I think
all tests are important, syntactical or otherwise. Its possible that not
all code is reachable (therefore testable) but the vision generally remains
at 100%.

Do let me know your view on this second point, so that I can remove these
tests if so required.

Robins Tharakan

> 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(at)cri(dot)ensmp(dot)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(at)cri(dot)ensmp(dot)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 6 July 2013 20:25, Robins Tharakan <tharakan(at)gmail(dot)com> wrote:

>
> Do let me know your view on this second point, so that I can remove these
> tests if so required.

Hi,

Please find attached the updated patch.

It address the first issue regarding reducing the repeated CREATE / DROP
ROLEs.

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.

--
Robins Tharakan

On Mon, Jul 15, 2013 at 10:23 AM, Robins Tharakan <tharakan(at)gmail(dot)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 (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.

--
Fabien.

On Mon, Jul 15, 2013 at 11:48 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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