Re: Add regression tests for ROLE (USER)

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