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

Search Discussions

  • Robins Tharakan at May 9, 2013 at 3:05 am
    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 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
  • Fabien COELHO at May 9, 2013 at 8:29 am
    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.
  • Robert Haas at Jul 3, 2013 at 3:51 pm

    On Thu, May 9, 2013 at 4:29 AM, Fabien COELHO 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
  • Robins Tharakan at Jul 7, 2013 at 3:26 am

    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
  • Robins Tharakan at Jul 15, 2013 at 2:23 pm

    On 6 July 2013 20:25, Robins Tharakan 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
  • Robert Haas at Jul 15, 2013 at 3:18 pm

    On Mon, Jul 15, 2013 at 10:23 AM, Robins Tharakan 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
  • Fabien COELHO at Jul 15, 2013 at 3:48 pm

    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.
  • Robert Haas at Jul 15, 2013 at 4:34 pm

    On Mon, Jul 15, 2013 at 11:48 AM, Fabien COELHO 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
  • Fabien COELHO at Jul 7, 2013 at 8:49 am

    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.
  • Tom Lane at Jul 7, 2013 at 3:12 pm

    Fabien COELHO 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
  • Andres Freund at Jul 7, 2013 at 3:52 pm

    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
  • Fabien COELHO at Jul 9, 2013 at 10:25 am

    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.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 19, '13 at 10:11p
activeJul 15, '13 at 4:34p
posts13
users5
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase